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

working on RunTimeWarning in tests #45

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Conversation

sbillinge
Copy link
Contributor

@sbillinge sbillinge commented Oct 29, 2024

first clean up applycutoff method to make its purpose clearer and to bring it closer to pep8 compliance.

then figure out how to work on the test warning

and to bring it closer to pep8 compliance
@sbillinge sbillinge changed the title cleaned up applycutoff method to bring make its purpose clearer working on RunTimeWarning in tests Oct 29, 2024
@sbillinge
Copy link
Contributor Author

@bobleesj @cadenmyers13 here is a PR which would be my first cut at addressing this warning.

First I try and understand what the function is supposed to be doing. It is basically creating a big array for the reciprocal space and then setting the voxels outside the measured Q-range to nan.

This is probably the desired behavior because they will always stay NaN. They may not stay zero if we set them to zero.

@sbillinge
Copy link
Contributor Author

I think the test is probably at fault then. It is a warning that is being thrown by numpy for some reason. We may want to try and catch that warning or handle it somehow in the test.

@sbillinge
Copy link
Contributor Author

we may be able to handle this by calling the test in a context manager, something like with self.assertWarns(RuntimeWarning):

@sbillinge
Copy link
Contributor Author

OK, so actually it is being thrown by another method in fourigui, intensity_upd_global() so we may want to figure out what that is doing and if we need to throw a warning. When we do that we can take the opportunity to turn those strings into f-strings....

For now I have to work on something else with the experiment, but maybe you guys can get the idea from what I did here and take it forward.... Let me know if you want me to merge these changes, which don't fix the problem but might be useful in your fix

Copy link
Contributor Author

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

see inline

sphere[mask] = np.nan
i, j, k = np.meshgrid(np.arange(xdim), np.arange(ydim), np.arange(zdim))
r2 = (i - xdim // 2) ** 2 + (j - ydim // 2) ** 2 + (k - zdim // 2) ** 2
mask = (r2 <= r2_inner) | (r2 >= r2_outer) # True if voxel is out of range
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if these should be < and > rather than <= and >= since it is tagging the unwanted regions as true. i.e., presumably Qmax is a valid value not an invalid value so we don't want to set Qmax to zero.

@sbillinge
Copy link
Contributor Author

Since we are moving away from unittest and to pytest, maybe we want to just junk this test and replace it with a new test that tests the behavior that we want?

@sbillinge
Copy link
Contributor Author

ok, a bit more digging and it is numpy.nanmax that returns the runtimewarning but this is desired behavior as we expect some slices to have all nan's given our function. So we would just like to figure out how to suppress this warning polluting our output. We could also just ignore it, but future selves will end up revisiting this again I am guessing, so if we can suppress it in the test, this would be desirable. I think we could also catch it and suppress it in the code as a, possibly better, alternative.

@sbillinge
Copy link
Contributor Author

Do you guys want to look into that?

@bobleesj
Copy link
Contributor

Will wait for @cadenmyers13 to first give his thought since it's his cookie

@cadenmyers13
Copy link
Contributor

cadenmyers13 commented Oct 29, 2024

@sbillinge Merging this will be fine, then I'll refer back to the comments. Thanks!

@sbillinge sbillinge merged commit 13a41a5 into diffpy:main Oct 30, 2024
3 checks passed
@sbillinge sbillinge deleted the runtimewarning branch October 30, 2024 03:25
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