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.

Sign up for GitHub

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

Jump to bottom

Change nan default fill_value for kerchunk arrays #255

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Change nan default fill_value for kerchunk arrays #255

wants to merge 9 commits into from
+41 −6

Conversation

mpiannucci
Copy link
Contributor

@mpiannucci mpiannucci commented Oct 11, 2024 •
edited
Loading

This removes adding nan as the fill_value for kerchunk references that incklude fill_value=null. This is necessary because only floats can have nan values represented

  • Closes #xxxx
  • Tests added
  • Tests passing
  • Full type hint coverage
  • Changes are documented in docs/releases.rst
  • New functions/methods are listed in api.rst
  • New functionality has documentation

@mpiannucci mpiannucci changed the title get rid of nan default fill value Change nan default fill_value for kerchunk arrays Oct 11, 2024
@TomNicholas
Copy link
Member

Would you mind adding a regression test? This was specifically spotted in the context of integer dtypes right?

Test failures are unrelated #249

@TomNicholas TomNicholas added the Kerchunk Relating to the kerchunk library / specification itself label Oct 11, 2024
@mpiannucci
Copy link
Contributor Author

Yeah. This weekend I'll handle if it's floats and add tests for that specifically

@TomNicholas
Copy link
Member

Yeah. This weekend I'll handle if it's floats and add tests for that specifically

Okay so I guess we'll wait for the tests to merge this then.

@mpiannucci
Copy link
Contributor Author

Lemme know if those tests are sufficient

@TomNicholas
Copy link
Member

The tests look good but something is failing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Kerchunk Relating to the kerchunk library / specification itself
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mpiannucci @TomNicholas