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

Fix crashes due to rounding errors in mean_occupation functions #1082

Merged
merged 4 commits into from
Aug 26, 2024

Conversation

johannesulf
Copy link
Contributor

This PR adds a check that the mean occupation for all halos is non-negative. If it's negative but reasonably close to 0 such that this may be caused by rounding errors, the mean occupation will be 0 exactly. If it's clearly negative, halotools will raise an error to alert the user that the model is unphysical. This should fix #1081.

@aphearin This PR also adds the above check for centrals and satellites. Let me know if this PR could produce any unwanted exceptions.

@johannesulf johannesulf changed the title fix for #1081 Fix crashes due to rounding errors in mean_occupation functions Jul 31, 2024
@aphearin
Copy link
Contributor

Thanks to @johannesulf and Kaustav for a simple fix of this subtle issue. I'm not sure why CI is failing at the moment, but it looks like it doesn't have anything to do with the code in the PR.

@aphearin
Copy link
Contributor

aphearin commented Aug 5, 2024

Well this workflow failure appears to be persistent (sometimes fails like this are only transient due to a temporary issue in the dependency chain). I found this github page indicating that the problem has something to do with an outdated workflow configuration, and so I tried simply updating checkout@v2 to checkout@v4 but that did not resolve the issue.

@aphearin
Copy link
Contributor

I'm having some trouble getting these workflows to pass, but there is nothing wrong with the source code in this PR - the test failures are due to a workflow config. I have a separate PR #1083 that will be used to address this, so this current PR can be merged once @johannesulf confirms that tests pass locally on his machine.

@johannesulf
Copy link
Contributor Author

johannesulf commented Aug 20, 2024

Sorry for taking a bit longer on this. I suspect that some of the workflow failures may be related to numpy 2. I get similar errors if I don't specify numpy==1.26.4. Ultimately, I was able to run the tests. Two small modifications to the tests had to be made.

  • One tests assumed that the first occupation moment can be None, initially. My code for mc_occupation didn't allow that. But since that behavior seems like a quirk, I just slightly changed the test so that the first occupation moment isn't None.
  • The recently released scipy 1.14 finally deprecated scipy.integrate.cumtrapz. So one test needed to be updated to accommodate that change.

@aphearin
Copy link
Contributor

Thanks for the leg work on code dependency issues. I'll try and solve the CI fails separately in #1083 but this looks good to me to go ahead and merge in to master.

@aphearin aphearin merged commit e8e623f into astropy:master Aug 26, 2024
1 of 5 checks passed
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.

Crash in mock generation due to rounding errors
2 participants