-
Notifications
You must be signed in to change notification settings - Fork 29
Use .sizes instead of .dims for xr.Dataset/xr.DataArray compatibility #71
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
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
d6606cf
Use .sizes instead of .dims for xr.Dataset/xr.DataArray compatibility
weiji14 f95d1c6
Do some pre-commit linting
weiji14 c465309
Merge branch 'main' into dims_to_sizes
weiji14 50f8b7d
Remove unused _as_xarray_dataset function
weiji14 f6a00e9
Merge branch 'main' into dims_to_sizes
weiji14 f0ebd20
FIx conversion to torch named tensors by changing frozen dict to tuple
weiji14 e110995
Fix KeyError for Pytorch tests
weiji14 954cd1d
Fix AttributeError on keras.py
weiji14 256de35
Mention that unit tests are only for DataArray and not Dataset
weiji14 f448a74
Remove concat_dim from CustomTFDataset to support xr.DataArray only
weiji14 9516190
Test torch.Tensor shapes to be more precise than just then batch size
weiji14 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change works because the unit tests in
test_torch_loaders.py
are actually testingxarray.DataArray
inputs only, and notxarray.Dataset
. Ideally there would be unit tests for bothxr.DataArray
andxr.Dataset
inputs, but this might expand the scope of the Pull Request a bit too much 😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this PR, the tests used
xarray.DataArray
inputs to the batch generator, butxarray.Dataset
inputs to the dataloaders since the batches were coerced into datasets. So I think this would be an additional breaking change in expectingxarray.DataArray
inputs.For the unit tests, I opened #83 to keep track of improvements for subsequent PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts on backwards compatibility here @weiji14? My impression is that the hardcoded
xarray.Dataset
variable names severely restricts the utility of the data loader. So, I think this is a worthwhile change since we'd be forced to break backwards compatibility anyways eventually for flexible variable names and that working from anxarray.DataArray
implementation is better. But we could add anif; else
block for dataset vs. dataarray if it's necessary to maintain the past behavior.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that we should try to be backward compatible and support both
xr.DataArray
andxr.Dataset
inputs. If you want, I can either:xr.Dataset
/xr.DataArray
cases in a separate PRThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for option 2