Fix crashes due to rounding errors in mean_occupation functions

astropy / halotools

Python package for studying large scale structure, cosmology, and galaxy evolution using N-body simulations and halo models

http://halotools.rtfd.org

BSD 3-Clause "New" or "Revised" License

104 stars 65 forks source link

Fix crashes due to rounding errors in mean_occupation functions #1082

Closed johannesulf closed 3 months ago

johannesulf commented 4 months ago

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.

aphearin commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.