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 warning for quantized variables #286

Merged

Conversation

JoelJaeschke
Copy link
Contributor

This lets a user know that the variable in question is stored as a quantized integer, but loaded as a float, which will cause the calculation of the bit-information to yield bogus results depending on the data. Fixes #202

JoelJaeschke and others added 2 commits July 30, 2024 23:26
…ariable in question is stored as a quantized integer, but loaded as a float, which will cause the calculation of the bitinformation to yield bogus results depending on the data.
@JoelJaeschke JoelJaeschke marked this pull request as draft July 30, 2024 21:44
@observingClouds
Copy link
Owner

Hi @JoelJaeschke,
Welcome 👋 . Thanks for the PR. At first sight it looks good. I will have to fix some tests upstream and will come back to you once that is done.
Cheers,
Hauke

@JoelJaeschke
Copy link
Contributor Author

Hi @observingClouds,

I did not run the test suite fully, so CI caught some stuff I failed to account for. I have pushed a new version that is more defensive in the way it does its checks. There are still two errors in the test I added when calling the Julia implementation, but I feel like those may be upstream errors? I don't have too much experience with how the code works yet, so forgive me if I have introduced those.

Best
Joel

@JoelJaeschke JoelJaeschke marked this pull request as ready for review July 31, 2024 09:37
@observingClouds
Copy link
Owner

I fixed some upstream issues that were caused by the usage of integers. The remaining failure of the CI can be traced back to milankl/BitInformation.jl#50.

You were "lucky" and hit some cases we haven't tested for before. We need to address these and can merge your contribution afterwards (hoping that no other issues occur).

Could you, @JoelJaeschke, in the meantime add a short description of your PR to the CHANGELOG.md? Thanks!

@observingClouds
Copy link
Owner

@JoelJaeschke it took a few iterations but with the upstream changes that I merged into this branch the tests now succeed 😄

@observingClouds observingClouds merged commit 1cdac23 into observingClouds:main Aug 15, 2024
10 of 11 checks passed
@observingClouds
Copy link
Owner

Congrats @JoelJaeschke on your first contribution to xbitinfo and welcome to the community 🎉 Thank you very much for your time and effort, looking forward to any future inputs!

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.

Flagging quantised input data formats
2 participants