Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Raise error when assigning to IndexVariable.values & IndexVariable.data #3862

Merged
merged 10 commits into from
Mar 23, 2020
Merged

Raise error when assigning to IndexVariable.values & IndexVariable.data #3862

merged 10 commits into from
Mar 23, 2020

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian requested a review from shoyer March 17, 2020 02:26
@max-sixty
Copy link
Collaborator

Great, good spot @dcherian

@dcherian dcherian mentioned this pull request Mar 19, 2020
Closed
13 tasks
@max-sixty
Copy link
Collaborator

OK to merge @dcherian ? We can almost release 0.15.1 then

@dcherian
Copy link
Contributor Author

I think @shoyer should confirm that this is what we want to do

dcherian
dcherian commented Mar 21, 2020
View reviewed changes
doc/whats-new.rst Outdated Show resolved Hide resolved
@keewis
Copy link
Collaborator

keewis commented Mar 22, 2020

this causes the docs to fail:

xarray/doc/plotting.rst

Line 660 in 0722b42

ds.w.values = [1, 2, 3, 5]

shoyer
shoyer approved these changes Mar 22, 2020
View reviewed changes
Copy link
Member

@shoyer shoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@dcherian
fix docs
104620a
@dcherian
update whats-new
705c7eb
@dcherian
Merge remote-tracking branch 'upstream/master' into indexvariable-values
8f464c4 
* upstream/master:
  add spacing in the versions section of the issue report (pydata#3876)
  map_blocks: allow user function to add new unindexed dimension. (pydata#3817)
  Delete associated indexes when deleting coordinate variables. (pydata#3840)
  remove macos build while waiting for libwebp fix (pydata#3875)
  Fix html repr on non-str keys (pydata#3870)
  Allow ellipsis to be used in stack (pydata#3826)
  Improve where docstring (pydata#3836)
  Add DataArray.pad, Dataset.pad, Variable.pad (pydata#3596)
  Fix some warnings (pydata#3864)
  Feature/weighted (pydata#2922)
  Fix recombination in groupby when changing size along the grouped dimension (pydata#3807)
  Blacken the doctest code in docstrings (pydata#3857)
  Fix multi-index with categorical values. (pydata#3860)
  Fix interp bug when indexer shares coordinates with array (pydata#3758)
  Fix alignment with join="override" when some dims are unindexed (pydata#3839)
@dcherian
Copy link
Contributor Author

I think the failed tests are unrelated since they only fail on the upstream-dev

FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[365_day]
FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[360_day]
FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[julian]
FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[all_leap]
FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[366_day]
FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[gregorian]
FAILED xarray/tests/test_cftimeindex.py::test_indexing_in_series_getitem[proleptic_gregorian]

@spencerkclark
Copy link
Member

I think now that you merged #3874, if you re-merge master they should pass.

@dcherian
Merge remote-tracking branch 'upstream/master' into indexvariable-values
20269ae 
* upstream/master:
  Re-enable tests xfailed in pydata#3808 and fix new CFTimeIndex failures due to upstream changes (pydata#3874)
@dcherian
Copy link
Contributor Author

Great. Thanks all.

Now to find out how many downstream projects test against master on CI :)

@dcherian dcherian merged commit 9eec56c into pydata:master Mar 23, 2020
@dcherian dcherian deleted the indexvariable-values branch March 23, 2020 13:42
dcherian added a commit to dcherian/xarray that referenced this pull request Mar 28, 2020
@dcherian
Merge remote-tracking branch 'upstream/master' into map-blocks-schema
4a355e6 
* upstream/master: (54 commits)
  Limit repr of arrays containing long strings (pydata#3900)
  expose a few zarr backend functions as semi-public api (pydata#3897)
  Use drawstyle instead of linestyle in plot.step. (pydata#3274)
  Implementation of polyfit and polyval (pydata#3733)
  misplaced quote in whatsnew (pydata#3889)
  Rename ordered_dict_intersection -> compat_dict_intersection (pydata#3887)
  Control attrs of result in `merge()`, `concat()`, `combine_by_coords()` and `combine_nested()` (pydata#3877)
  xfail test_uamiv_format_write (pydata#3885)
  Use `fixes` in PR template (pydata#3886)
  Tweaks to "how_to_release" (pydata#3882)
  whatsnew section for 0.16.0
  Release v0.15.1
  whatsnew for 0.15.1 (pydata#3879)
  update panel documentation (pydata#3880)
  reword the whats-new entry for unit support (pydata#3878)
  Raise error when assigning to IndexVariable.values & IndexVariable.data (pydata#3862)
  Re-enable tests xfailed in pydata#3808 and fix new CFTimeIndex failures due to upstream changes (pydata#3874)
  add spacing in the versions section of the issue report (pydata#3876)
  map_blocks: allow user function to add new unindexed dimension. (pydata#3817)
  Delete associated indexes when deleting coordinate variables. (pydata#3840)
  ...
@rsokl
Copy link

rsokl commented Apr 10, 2020

@dcherian @max-sixty is there a reason why this change wasn't incorporated into values.setter? This change blocks the ability to update coordinate values in-place. This breaks code in a more fundamental way than just requiring users to rely on assign_coords - it can necessitate a substantial change in code design for people who were relying on the ability to mutate coordinates. This also leads me to ask: was it really appropriate to call such a substantial breaking change only a patch?

@shoyer
Copy link
Member

shoyer commented Apr 10, 2020

is there a reason why this change wasn't incorporated into values.setter?

I'm not quite sure what you mean here. That's exactly what this change does, making assignment to IndexVariable.values an error.

If you were mutating .values on an indexed variable, then you already had a likely bug, because only part of xarray's data structures were being updated (the Variable, but not indexes). That's why we put this in as a bug fix -- see the linked issue #3470 for examples.

Unfortunately we couldn't figure out another way to make this work -- see #3470 (comment) for discussion. We're definitely open to alternatives if you can come up with them, though!

@rsokl
Copy link

rsokl commented Apr 10, 2020

That's exactly what this change does, making assignment to IndexVariable.values an error.

Sorry, I got a bit criss-crossed between this PR and the thread in #3470.

In light of #3470 (comment) it seems like the next best thing would be to given assign_coords the optional ability to update the DataArray/DataSet in-place. Thoughts?

@shoyer
Copy link
Member

shoyer commented Apr 10, 2020 •
edited
Loading

In light of #3470 (comment) it seems like the next best thing would be to given assign_coords the optional ability to update the DataArray/DataSet in-place. Thoughts?

There's no need to use assign_coords. You can also just use normal assignment to a DataArray/Dataset or coords, e.g,

ds['time'] = new_values  # good
ds.coords['time'] = new_values  # good
ds.coords['time'].values = new_values  # broken, disabled by this PR
ds.coords['time'].values[:] = new_values  # also buggy (but harder to disable)

This would probably be worth clarifying in the error message.

(Under the hood, assign_coords basically does the exactly same thing as assignment to coords, it just copies the Dataset/DataArray` first)

@rsokl
Copy link

rsokl commented Apr 10, 2020

Ah! Nice! Yeah the error message led me to believe that there was no path for in-place updates.

douglatornell added a commit to SalishSeaCast/SalishSeaNowcast that referenced this pull request Jul 9, 2020
@douglatornell
Fix UTC offset calc VFPA 2nd Narrow HADCP dataset
3286cd5 
This addresses a breaking change in xarray=0.15.1
(see pydata/xarray#3862).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shoyer shoyer shoyer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

warn when updating coord.values : indexes are not updated
6 participants
@dcherian @max-sixty @keewis @spencerkclark @rsokl @shoyer