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 nan warning in get_bitinformation; Closes #200 #260

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

Conversation

thodson-usgs
Copy link

Let's start by closing the issue on warnings, but I think the implementations could be rewritten to handle NaN's, which would be good for masked datasets.

@thodson-usgs
Copy link
Author

I apologize, my organization is swapping SSL certs and that seems to be preventing me from running the unit test, but this PR is straightforward.

@observingClouds
Copy link
Owner

Thanks for the PR. First time contributors cannot run the tests automatically. We allow those manually. So no issue on your side.

Copy link
Owner

@observingClouds observingClouds left a comment

Choose a reason for hiding this comment

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

Could you please also add a line to CHANGELOG.md? Thanks!

xbitinfo/xbitinfo.py Outdated Show resolved Hide resolved
@milankl
Copy link
Collaborator

milankl commented Feb 9, 2024

Ideally NaNs should be masked and then ignored in the bit pair count algorithm so that in a bitstream of 1 0 NaN 0 1 it would only count two bit pairs 10 and 01 instead of counting across the bits of the NaN (which is a valid operation as it's also just bits, but not really meaningful). This is how I've implemented it in BitInformation.jl

https://github.com/milankl/BitInformation.jl/blob/953dac3b5ae12a719456f1be3ae0ef4317f24e86/src/bit_count.jl#L127-L131

although NaN's are not automatically converted to a mask at the moment, but that could easily be added. Well maybe requires some work to avoid a double pass of the data, but yes.

@thodson-usgs
Copy link
Author

Changelog updated.
And I meant that the SSL was preventing me from running tests locally. I don't fully understand why, though I could see this going awry if Julia, pip, conda aren't all configured correctly...But that's for me to worry about.

@thodson-usgs
Copy link
Author

Well, this is awkward. xb.get_bitinformation calls itself such the warning is thrown at least twice (+1 per additional dim). Need to ponder the simplest way to avoid that.

@thodson-usgs
Copy link
Author

Eh, I don't particularly like my solution, which just loosens the unit test to permit the multiple warnings. The alternative is to import some other module that let's us inspect the calling function to decide whether or not to throw the warning. Let me know if you'd prefer that.

@observingClouds
Copy link
Owner

Well, this is awkward. xb.get_bitinformation calls itself such the warning is thrown at least twice (+1 per additional dim). Need to ponder the simplest way to avoid that.

Oh, that is unfortunate. Let me fix this in #261.

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.

3 participants