issue_comments

Thanks @dnowacki-usgs

Hello @dnowacki-usgs! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2020-08-19 19:20:14 UTC

I've opened a PR to update the guide, hopefully that helps.

Ah, I see, merge instead of rebase. I'll try that next time.

I think it should be sh git checkout master git pull git checkout <feature-branch> git merge master git push or sh git fetch upstream git merge upstream/master git push instead

ah, now I understand where the duplication is coming from. I'm guessing you did that and then tried to push, but it complained about different commits, so you pulled and then pushed.

I fixed it for you.

I think we should update that guide, rebasing is usually not a good idea because then you'd have to force-push, too (and github doesn't handle force-pushing that well).

Ugh, sorry about that. I'm following the instructions in contributing to xarray. It's a fetch/rebase. Happy to do it another way if there is a better practice. Thanks.

which command did you use to create the merge? I think that was some sort of rebase merge, which means that all merged commits are included in the commit history, and github doesn't seem to work well with this kind of merges. You also have duplicated commits in the history, which confuses me.

Edit: it seems you somehow merged / rebased the branch onto itself. Just as reference, I usually use sh git checkout master git pull git checkout <feature-branch> git merge master git push for merging

could you merge in master? I think that would fix the failing build.

Updated the try/except following @keewis's lead. Also sparse 0.11.0 has been released so I was able to check that the warning is correctly issued for 0.10.0, then I upgraded, and the warning is no longer issued (and sparse doesn't choke on the casting kwarg).

I just saw I was up for reviewing this — sorry for missing that.

Looks good. @keewis 's suggestion for the back-compat code looks like a nice refinement — it includes a version check which will ease disentangling it. Could we add that and then merge?

I think that was due to a rebase merge (?). I fixed the commit history using cherry-picking and force-pushed to your branch, but there's a backup of the old branch if something went wrong.

@dnowacki-usgs, if you want to add new commits you'll first have to force-pull (or delete your local branch and check out the remote branch – just make sure that you don't lose any unpushed commits).

Thanks for the review @dcherian. I modified the tests as suggested and added some docstring updates. Let me know what you think.

There are some duplicated commits — 3x Recreate gaj... — though still weird that GH and git disagree.

I'd suggest the squash and push -f...

* e23529e0 - (HEAD -> astypekeepattrs, dnowacki-usgs/astypekeepattrs, pr/dnowacki-usgs/4314) Merge branch 'astypekeepattrs' of https://github.com/dnowacki-usgs/xarray into astypekeepattrs (3 hours ago) <Dan Nowacki> |\ | * ac42be80 - improve error handling and check if sparse array (6 days ago) <Dan Nowacki> | * e2d38550 - fix dtype test (7 days ago) <Dan Nowacki> | * e079cf86 - Merge branch 'astypekeepattrs' of https://github.com/dnowacki-usgs/xarray into astypekeepattrs (7 days ago) <Dan Nowacki> | |\ | | * 1296f590 - Merge branch 'master' into pr/dnowacki-usgs/4314 (8 days ago) <Maximilian Roos> | | |\ | | * | 5c101d85 - Accept keep_attrs flag, use apply_ufunc in variable.py, expand tests (8 days ago) <Dan Nowacki> | | * | d75e1fb9 - Recreate @gajomi's #2070 to keep attrs when calling astype() (8 days ago) <Dan Nowacki> | * | | eb86c8a6 - add to whats new (7 days ago) <Dan Nowacki> | * | | db014baf - Ignore casting if we get a TypeError (indicates sparse <= 0.10.0) (7 days ago) <Dan Nowacki> | * | | 01d1b378 - Accept keep_attrs flag, use apply_ufunc in variable.py, expand tests (7 days ago) <Dan Nowacki> | * | | 00bfeaa3 - Recreate @gajomi's #2070 to keep attrs when calling astype() (7 days ago) <Dan Nowacki> * | | | dba93b18 - improve error handling and check if sparse array (3 hours ago) <Dan Nowacki> * | | | 4856eec5 - fix dtype test (3 hours ago) <Dan Nowacki> * | | | 983f17d5 - add to whats new (3 hours ago) <Dan Nowacki> * | | | 9f9b06c6 - Ignore casting if we get a TypeError (indicates sparse <= 0.10.0) (3 hours ago) <Dan Nowacki> * | | | a277c2a4 - Accept keep_attrs flag, use apply_ufunc in variable.py, expand tests (3 hours ago) <Dan Nowacki> * | | | 833e444f - Recreate @gajomi's #2070 to keep attrs when calling astype() (3 hours ago) <Dan Nowacki> * | | | 7ee97989 - Accept keep_attrs flag, use apply_ufunc in variable.py, expand tests (4 hours ago) <Dan Nowacki> * | | | dd4f5494 - Recreate @gajomi's #2070 to keep attrs when calling astype() (4 hours ago) <Dan Nowacki> * | | | 7daad4fc - (upstream/master, upstream/HEAD, origin/master, master) Implement interp for interpolating between chunks of data (dask) (#4155) (2 days ago) <Alexandre Poux> * | | | d514b120 - Add @mathause to current core developers. (#4335) (2 days ago) <Deepak Cherian> * | | | ceadecd5 - install sphinx-autosummary-accessors from conda-forge (#4332) (3 days ago) <keewis> * | | | 1791c3b6 - Use sphinx-accessors-autosummary (#4323) (4 days ago) <keewis> * | | | df7b2eae - ndrolling fixes (#4329) (4 days ago) <Keisuke Fujii> * | | | f02ca537 - DOC: fix typo argmin -> argmax in DataArray.argmax docstring (#4327) (5 days ago) <Sander> * | | | 6e9edff5 - pin sphinx to 3.1(#4326) (5 days ago) <keewis> * | | | 1d3dee08 - nd-rolling (#4219) (6 days ago) <Keisuke Fujii> |/ / / * | | e04e21d6 - Implicit dask import 4164 (#4318) (7 days ago) <inakleinbottle> * | | 7ba19e12 - allow customizing the inline repr of a duck array (#4248) (7 days ago) <keewis> | |/ |/| * | 98dc1f4e - silence the known docs CI issues (#4316) (8 days ago) <keewis> * | ed6fff2c - enh: fixed #4302 (#4315) (8 days ago) <Nick R. Papior> * | 1101eca6 - Remove all unused and warn-raising methods from AbstractDataStore (#4310) (8 days ago) <alexamici> |/ * e1dafe67 - (dnowacki-usgs/master) Fix map_blocks example (#4305) (10 days ago) <Tom Augspurger>

The weirdest part is that the local diff is fine; here's the output of git --no-pager diff upstream/master. Is it maybe because one of the merge commits was mine rather than @dnowacki-usgs ?

Sorry if I contributed to this.

One override is to apply this patch on master and force push this branch. I'm happy to do it if @dnowacki-usgs agrees.

Weird - it got even worse. I can try tomorrow morning (European time) unless @max-sixty wants to have a go.

Did a fetch and rebase, not sure why commits from other PRs added to master in the meantime show up in this one 🤷

Somehow the contents of #4248 got in here. Can you try to merge master and see if they disappear?

Great — I think this looks good — any final thoughts?

I think that's a false positive, our flake8 and black CI don't report that. I guess the bot will remove that FileNotFoundError report once you push a new commit, so it should be safe to ignore it.

I think(?) the pep warnings are from the commits @max-sixty pushed several days ago during testing

Maybe I am overcomplicating things but I came up with this solution:

Catch the error. If it is the unallowed casting kwarg error, and we are dealing with a sparse array, pop the casting kwarg and issue a RuntimeWarning that we are doing so. Otherwise, raise the original error since it might be something else non-sparse related.

Does that seem reasonable?

I'm fine with reraising — though I think we should reraise, otherwise isn't it a confusing message?

The try-expect is probably fine given the narrowness (very open to holding too low a standard tho); one step up would be checking it is a spare array.

My solution of try/except and popping casting from kwargs felt a bit kludgey, so I'm definitely open to other ideas. Letting it raise and xfailing the test would mean that we can no longer perform astype on sparse arrays until a new sparse version is released in the future. Though I suppose that would depends on the next xarray vs sparse release timeline.

I wonder if we should just let it raise (but xfail our test on sparse < 0.10.0). This is an "imperfect NEP-18 implementation" issue and it isn't feasible to catch all such errors.

For sure — good idea to defer to numpy rather than raise — should we do that only on existing versions though? Otherwise we won't roll over to the new version when they do release their next version? (the version check can be a single line, there are lots of examples in the repo, lmk if that's not clear)

Nice! Thanks a million @dcherian for raising that issue over at sparse.

@max-sixty regarding the minimum version, it seems like sparse has a relatively infrequent release cycle (many months) so it could be a while before we have a minimum version to require. Could we check whether we are dealing with a sparse array and call the numpy method instead for that use case, until sparse supports it?

What do you think about reraising the error with a message about upgrading? Is this a narrow enough use case that we can avoid backward-compat fix in xarray?

well that was fixed quickly! Now we need to think of backcompat workarounds.

I've opened an issue at sparse. Let's see what the outcome is.

Looks like COO.astype doesn't accept the casting kwarg: https://sparse.pydata.org/en/stable/generated/sparse.COO.astype.html

I'm not sure re the test at first glance, I don't see how those tests are related, but master seems to pass. I pushed a merge to master as a check — hope that's OK. Let's see if that sheds any light

Thanks @max-sixty. I pushed a second commit, including adding a keep_attrs argument so people can keep the old behavior of losing the attrs. I added a bit more testing on for general astype behavior, checking dtypes are correct and values are similar.

What do you make of the failing tests in test_sparse.py (test_dataarray_method and test_variable_method)? I'm not really familiar enough with the pytest parametrize syntax to figure out what's going on.