Update GINI Xarray support #2635
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dopplershift
added
Type: Enhancement
dopplershift
force-pushed
the
gini-backend
branch
from
b8dbef7 to
774a6a0
Compare
Member
Author
|
@kgoebber You may be interested in the |
dopplershift
force-pushed
the
gini-backend
branch
2 times, most recently
from
b5ddf08 to
5d4ba44
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| def test_gini_xarray_entrypoint(filename, bounds, data_var, proj_attrs, image, dt, | ||
| specify_engine): | ||
| """Test that GINI files can be opened directly by xarray using the v2 API.""" | ||
| ds = xr.open_dataset(get_test_data(filename, as_file_obj=specify_engine), |
Check warning
Code scanning / CodeQL
File is not always closed
| """Test that add_timestamp can work with xarray datetime accessor.""" | ||
| fig = plt.figure() | ||
| ax = fig.add_subplot(1, 1, 1) | ||
| ds = xr.open_dataset(get_test_data('AK-REGIONAL_8km_3.9_20160408_1445.gini'), |
Check warning
Code scanning / CodeQL
File is not always closed
dcamron
previously approved these changes
Oct 4, 2022
Member
dcamron
left a comment
dcamron
left a comment
There was a problem hiding this comment.
Tests fine to me, and I'm alright with the decisions here re old xarray API and directly using GiniFile. Nothing immediately jumps out as missed here. If we need I can leave this up until the morning in case anyone else has last-minute considerations, but I think it's good to go!
This refactors a bit to make this reasonably direct, rather than having it re-parse all the variables created by the previous support. It also makes some minor tweaks to the general xarray decoding by no-longer using masked arrays (Xarray uses nans) and adds lat/lon to coordinates--as they should have been in the first place.
All that's needed is a conditional call to .item(), and everything "just works".
dopplershift
force-pushed
the
gini-backend
branch
from
565cf37 to
a949336
Compare
Member
Author
|
Resolved the conflict, this should be good to go. |
dcamron
approved these changes
Oct 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description Of Changes
This adds xarray direct reading of GINI files by implementing v2 of xarray's backends API. This registers a new entrypoint that basically allows
open_dataset()to just work™️ for any file with a name that ends with ".gini". It also works to specifyengine='gini'--and it all works without needing to manually import metpy thanks to the entrypoints mechanism.The v2 backend API was added in xarray 0.18.0, released 6 May 2021. I've elected at this point not to bother making the imports and classes conditional. At least right now I didn't feel like the effort was worth it to cover 8 more months of xarray releases.
Refactored a bit, but it could be cleaned up in the future when we can drop the previously supported mechanisms. This tweaks the xarray reading a bit to include lat/lon in coordinates (should have already been done) and eliminates using a masked array (xarray is supposed to use nans).
As part of this I also added support for the xarray
.dtaccessor toadd_timestamp--it already worked with it in calling.strftime, this just makes it try.item()if the result of that call tostrftimeisn't a string.Also updated the existing GINI example to use all of this stuff.
Checklist