-
Notifications
You must be signed in to change notification settings - Fork 35
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
Regridded output now same dtype as input #102
Conversation
This fails without update because passed in data was int. In past this would have output float64 but output is not int, so test was expecting higher precision. Simple work around is to make input same dtype as we expect output to be.
I think this is ready for review! I'd especially appreciate input in how this is throwing a Also, the change I needed to make to the pre-existing unit test makes me a bit nervous because this PR is clearly breaking from past behavior. Just another thing to note. Anyone else nervous about this? |
I think I could use maintainer input on this. Specifically, I'm looking at
from https://github.com/pangeo-data/xESMF/pull/102/checks?check_run_id=3091031309. This PR hasn't added or changed dependencies. I fixed other apparent linting issues by deactivating |
@brews please feel free to add this should take care of the linting test |
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.
sorry, this is not necessary anymore, fixed in master
|
the handling in the dataset is finicky and decisions need to be made in terms of desired behavior. |
Change from reviewer. toml is no longer required for blockdoc linting.
This fails without update because passed in data was int. In past this would have output float64 but output is not int, so test was expecting higher precision. Simple work around is to make input same dtype as we expect output to be.
Change from reviewer. toml is no longer required for blockdoc linting.
@raphaeldussin Could you clarify what needs to be done to merge this ? What decisions do we need to make? |
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.
I'd recommend updating PR with modifs from here brews#2
Thanks for the bump, @huard. I merged in the suggested corrections from @raphaeldussin. I agree with @raphaeldussin that accepting multiple dtypes from Datasets is ideal behavior. It appears to work in testing so 👍 . Pending CI/CD checks, I think this is ready for re-review, @raphaeldussin. |
LGTM |
Output regridded data is now the same type as input data. If input is
xarray.Dataset
with multiple unique dtypes, raisesValueError
.PR includes integration tests for new behavior under different input types/containers.
One pre-existing test needed to be updated because it output
int
s when the test was expecting precision fromfloat
.Close #99.