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

Condition to handle NaNs #44

Closed
wants to merge 2 commits into from
Closed

Conversation

cadenmyers13
Copy link
Contributor

I had a chat with Bob this morning about the RuntimeWarning. The goal of the test is to verify that the applycutoff method is working as expected for a given qmin and qmax. We agreed that the test was okay. We came up with two possible fixes for this through modification of the source code.

  1. If a slice of all NaNs is encountered, set all those NaNs to zero. This we thought to be simpler and is the solution in this PR.
  2. Find a different way to represent NaN values in the data such as replacing all NaNs with zeros. This seems to have a lot of moving parts and could cause more trouble.

The below images are from the GUI where all NaNs are present and where some NaNs are present (for reference). These images were taken after applying the edits made on this PR. @sbillinge does this seem valid to you?

Screenshot 2024-10-28 at 11 01 38 AM
Screenshot 2024-10-28 at 11 01 51 AM
Screenshot 2024-10-28 at 11 02 01 AM


if np.all(np.isnan(plane)):
plane = np.zeros_like(plane)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

condition setting all NaNs to zero

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do that, that means we are simply converting nan values to all 0s. but only for [nan, nan, ... , nan] but not when [nan, 0.1, 0.2, nan, ...]

so doesn't feel like a good solution to me - when we call np.nanmax, I think it will pick up 0 instead of nan and this might not be good since your code has a section where it calculates nan_ratio.

I am not sure why we use nan in the first place. Why nan instead of 0? Having nan around for an array where it should have the same type for the particular array normally, it feels quite odd.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not the preferred behavior. Setting NaN's to 0 falls under the umbrella of "magic". Especially as scientists we want to avoid updating people's data.

Caden said "The goal of the test is to verify that the applycutoff method is working as expected for a given qmin and qmax." which of course is a tautology. That is what a test does. But the question is what do "we expect" that function to do. In other words, what is our desired behavior?

@cadenmyers13 cadenmyers13 deleted the nans branch October 30, 2024 14:57
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