Relax requirement for consistency between halo mass and radius
Opened this issue · 5 comments
Thanks to @jmsull and @rainwoodman for pointing out the problem this is causing in bccp/nbodykit#634.
@jmsull - yes, sorry, I forgot to mention that the fix requires you to pass in a halo_boundary_key.
@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!
@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.