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

add support for NaN fill values #149

Merged
merged 4 commits into from
Jan 24, 2020
Merged

Conversation

gdkrmr
Copy link
Contributor

@gdkrmr gdkrmr commented Jan 23, 2020

  • Allows for using NaN, Inf and -Inf as fill values. Tested with my R wrappers only for NaN, I will test it for Inf tomorrow. Using NaN as fill value #148
  • This PR also fixes a small bug (?): The raw compressor was missing from one of the maps between the compressor type and the string value.
  • Not sure which NaN value to use, currently using std::numeric_limits<double>::quiet_NaN().
  • No new unit tests.
  • I cannot run the tests on my computer, because my OS is too old, so I simply transferred all the changes between files and did not run the tests.

Copy link
Owner

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this and the changes look good to me.
For some reason it fails a few tests though.
I will investigate this later and let you know what I find.

Copy link
Owner

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one issue in the changes, I have fixed it in gdkrmr#1 and also
fixed another issue with the fillvalue and added a python unittest.
Once you merged that one, we can go ahead with this PR :).

Copy link
Owner

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests pass (never mind appveyor, the tests always fail on PRs for some reason) now and this looks good to me. Let me know if everything works out with your tests for +/- inf, then I will merge.

@gdkrmr
Copy link
Contributor Author

gdkrmr commented Jan 24, 2020

Confirmed: Works from my side from R for NaNs, NAs, Inf, and -Inf 👍. I think this is ready to merge!

@constantinpape
Copy link
Owner

Thanks for the contribution :).

@constantinpape constantinpape merged commit 3eaa042 into constantinpape:master Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants