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.

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

Quick fix for test_narr_example_variable_without_grid_mapping test (broken by xarray v0.14) #1206

Merged
merged 1 commit into from Oct 16, 2019
Merged

Quick fix for test_narr_example_variable_without_grid_mapping test (broken by xarray v0.14) #1206

merged 1 commit into from Oct 16, 2019

Conversation

jthielen
Copy link
Collaborator

@jthielen jthielen commented Oct 15, 2019

With xarray v0.14 (in particular, after pydata/xarray#3234), MetPy's usage of xarray.Dataset.apply in parse_cf results in a duplicated coordinate issue, like that observed in 2fe00b6. It seems like it is a very subtle index issue with xarray, however, it can be avoided entirely through using xarray.merge instead. The likely upstream issue remains, but this at least fixes MetPy for now.

With xarray v0.14 (in particular, after xarray issue 3234), MetPy's usage of xarray.Dataset.apply results in a duplicated coordinate issue, like that observed in 2fe00b6. It seems like it is a very subtle index issue with xarray, however, it can be avoided entirely through using xarray.merge instead. The likely upstream issue remains, but this at least fixes MetPy for now.
@jthielen jthielen mentioned this pull request Oct 15, 2019
3 tasks
@zbruick zbruick added this to the 0.11.1 milestone Oct 15, 2019
@zbruick zbruick added Area: Xarray Pertains to xarray integration Type: Bug Something is not working like it should labels Oct 15, 2019
Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

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

Pending CI. This will fail on the lack of fixing the Gini files here that #1204 does. I'll let @dopplershift handle merging these, as it will require one of them to go in without Travis passing and then a rebase.

@jthielen
Copy link
Collaborator Author

I think I finally figured out what originally went wrong: the unit_array setter on the MetPyDataArrayAccessor does not account for updating indexes. Fortuitously, before pydata/xarray#3234, the DataArray._indexes attribute was not yet modified, so the indexes were only created after the coordinate scaling. However, that seems to have changed with pydata/xarray#3234 when using apply. So, in order to not have apply attempt to combine both unscaled and scaled indexes (which is what ended up happening), we need to somehow "reset" the indexes on each variable to their scaled values.

This means an alternative fix to using merge here would be to add reset_index to the function that is applied in apply. If that fix is preferred, I can modify this PR to do that instead.

Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

I like this approach because it reduces the unique code paths at play here. It really goes now: "If no variables requested, do them all through the same path as we would have done."

@dopplershift dopplershift merged commit e3137a9 into Unidata:master Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Xarray Pertains to xarray integration Type: Bug Something is not working like it should
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants