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

Relax requirement for consistency between halo mass and radius #993

Open
aphearin opened this issue Aug 24, 2020 · 5 comments · Fixed by #995
Open

Relax requirement for consistency between halo mass and radius #993

aphearin opened this issue Aug 24, 2020 · 5 comments · Fixed by #995

Comments

@aphearin
Copy link
Contributor

Thanks to @jmsull and @rainwoodman for pointing out the problem this is causing in bccp/nbodykit#634.

@aphearin
Copy link
Contributor Author

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

@aphearin aphearin reopened this Sep 18, 2020
@jmsull
Copy link

jmsull commented Sep 26, 2020

@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
Copy link
Contributor Author

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

@jmsull
Copy link

jmsull commented Jan 28, 2021

@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 added a commit to aphearin/halotools that referenced this issue Feb 19, 2021
aphearin added a commit to aphearin/halotools that referenced this issue Feb 19, 2021
@aphearin
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants