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

Retain original _FillValue in encoding #711

Merged
merged 4 commits into from
Mar 18, 2024

Conversation

maxrjones
Copy link
Contributor

This is a follow-up to #471 that adds _FillValue to the retained encoding options. Based on #471 (comment), _FillValue was initially excluded because it causes the tests to fail when asserting that the expected and actual schemas are equal. That fails because it's common to specify _FillValue as np.nan and nans do not equal each other. This PR adds a _assert_schema_equal that will treat NaNs in the schema as equal, so that the original _FillValue can be retained.

This PR is a prerequisite to #669 because we need to make all encoding options stored in .zmetadata follow valid JSON.

@norlandrhagen norlandrhagen added the test-integration Apply this label to run integration tests on a PR. label Mar 18, 2024
maxrjones and others added 2 commits March 18, 2024 18:34
Co-authored-by: Raphael Hagen <norlandrhagen@gmail.com>
@cisaacstern
Copy link
Member

Thanks @maxrjones! Based on discussion following #471 (comment), it does not seem as though there was a specific justification for excluding _FillValue initially, aside from the testing issues resolved by this PR. This track with what you shared in the meeting today! So in principle, this is good by me. I'll just wait to make sure tests pass.

@cisaacstern cisaacstern merged commit ae64a98 into pangeo-forge:main Mar 18, 2024
9 checks passed
@cisaacstern
Copy link
Member

Thanks! @norlandrhagen please feel free to release per https://pangeo-forge.readthedocs.io/en/latest/contributing.html#releasing if it's useful for you all!

@maxrjones maxrjones deleted the fill-value branch March 18, 2024 23:43
@maxrjones
Copy link
Contributor Author

thanks for the prompt reviews @cisaacstern and @norlandrhagen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test-integration Apply this label to run integration tests on a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants