search

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

Relax requirement for consistency between halo mass and radius #993

Open aphearin opened 4 years ago

aphearin commented 4 years ago

Thanks to @jmsull and @rainwoodman for pointing out the problem this is causing in https://github.com/bccp/nbodykit/issues/634.

aphearin commented 4 years ago

@jmsull - would you mind letting me know whether #995 resolves the issue for you?

jmsull commented 4 years ago

@aphearin Sorry it does not seem to resolve the issue - without changing anything in the example code I get the same error as before (screenshot 1). I tried running the test suite and the new test in #995 seems to pass (screenshot 2). Do I need to explicitly pass halo_boundary_key somehow?

Screen Shot 2020-09-26 at 02 30 40 Screen Shot 2020-09-26 at 02 35 26
aphearin commented 4 years ago

@jmsull - yes, sorry, I forgot to mention that the fix requires you to pass in a halo_boundary_key.

jmsull commented 3 years ago

@aphearin Sorry for the extreme lag on this. This did not end up working for me at the time. I tried passing halo_boundary_key kw through nbodykit (and through halotools alone) to no avail. I recently worked around the issue by just commenting out the line Yu mentioned that specifies halo_mvir and halo_rvir as the default values at the top of model_defaults.py in halotools. On the nbodykit side there appear to be no changes needed once this is done, and the correct mdef columns (either halo_m/r200m or halo_m/rvir) appear to work fine.

Here is my attempt at seeing where this comes from: The model_defaults (that include mvir,rvir) are still enforced by this copy action here. Then when the mock factory is created and tries to call preprocess_halo_catalog(), this line fails to find the halo_rvir column. It doesn't actually need this, but it thinks it does because it has inherited the model_defaults. I tried printing the original halo keys before adding the additional_haloprops keys to confirm this - when the line is uncommented the original keys have halo_m/rvir (even if the original table was halo_m/r200m) and I get the error. If the line is commented out then the halo_m/rvir is not in the original keys (as wanted) and the m/r200m are added in additional_haloprops keys. I don't have sufficient perspective on the halotools design to fully explain this, but hopefully this helps!

aphearin commented 3 years ago

@jmsull - thanks for the patience on this. Your notes on tracking down the issue were extremely helpful so special thanks for those. I finally started making some headway on this fix, which turned out to be more subtle than I had previously thought. I ran out of time today but should hopefully finish soon.