Let TensorFlow accessor and data loader handle either xarray.DataArray or xarray.Dataset inputs #107
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #107 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 182 190 +8
Branches 33 35 +2
=========================================
+ Hits 182 190 +8 ☔ View full report in Codecov by Sentry. |
xbatcher/accessors.py
Outdated
| @xr.register_dataarray_accessor("keras") | ||
| @xr.register_dataset_accessor("keras") |
There was a problem hiding this comment.
Sure that we want to use keras instead of tensorflow as the accessor name (considering that the returned tensors are tf.Tensor objects)?
There was a problem hiding this comment.
Oh wait, I guess we're using xbatcher.loaders.keras.CustomTFDataset in the dataloader at https://github.com/xarray-contrib/xbatcher/blob/ed45a99da54503de2e94cc90f12510f590ea9be6/doc/api.rst#dataloaders, so might as well stick with keras to be consistent (unless anyone is keen to change everything to tensorflow).
There was a problem hiding this comment.
I agree with your point. What do you and @norlandrhagen think of tf for the accessor name and TFAccessor as the class name to keep it shorter?
There was a problem hiding this comment.
Thanks for the feedbaack @weiji14 @maxrjones
Good point on the naming. I can update the accessor name and class name. Should up update the data loader naming(xbatcher.loaders.keras.CustomTFDataset) in this PR?
There was a problem hiding this comment.
Let's rename the tensorflow dataloader in a separate PR (so that it shows up in the changelog as a backward incompatible change).
xbatcher/accessors.py
Outdated
| @xr.register_dataarray_accessor("keras") | ||
| @xr.register_dataset_accessor("keras") |
There was a problem hiding this comment.
I agree with your point. What do you and @norlandrhagen think of tf for the accessor name and TFAccessor as the class name to keep it shorter?
weiji14
changed the title
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
maxrjones
changed the title
Description of proposed changes
Adds to: #83 (comment). Based off changes in PR: #85.
_as_xarray_dataarraymoved outside ofTorchAccessorclass._as_xarray_dataarraymethod modified to useis_instanceinstead of try:except._as_xarray_dataarrayKerasAccessorand corresponding tests.