halotools icon indicating copy to clipboard operation
halotools copied to clipboard

Relax requirement for consistency between halo mass and radius

Open aphearin opened this issue 5 years ago • 5 comments

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

aphearin avatar Aug 24 '20 13:08 aphearin

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

aphearin avatar Sep 18 '20 16:09 aphearin

@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

jmsull avatar Sep 26 '20 16:09 jmsull

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

aphearin avatar Sep 26 '20 17:09 aphearin

@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 avatar Jan 28 '21 16:01 jmsull

@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.

aphearin avatar Feb 19 '21 23:02 aphearin