Skip to content

Adding BGS filter to prepare_sim#212

Open
SBouchard01 wants to merge 8 commits into
abacusorg:mainfrom
SBouchard01:bgs_prep
Open

Adding BGS filter to prepare_sim#212
SBouchard01 wants to merge 8 commits into
abacusorg:mainfrom
SBouchard01:bgs_prep

Conversation

@SBouchard01

Copy link
Copy Markdown
Contributor

At Sandy's suggestion, I had a custom filter on the halos & particles subsamples to allow the BGS density to be reached in my branch of the package (it exists in the current file, but commented out). Given the recent changes in prepare_sim that need to re-run it, I propose an implementation of that filter as an option in the official pipeline.

This branch also replaces most of the print statements by logger calls for clarity and stdout buffering delays in prepare_sim and menv.

Open questions & remarks:

  • 2 alternate functions of subsample_halos and submask_particles are commented out in the code. They are functionally the same as the current ones but with some magic values changed.
  • By curiosity, submask_particles has an extra line when calling the MT (or BGS) filter: ntarget = np.minimum(ntarget, 100) that is not called for the ST usual filter. Is that intentional ? If yes, why ? If not, the statement can be moved before the submask initialization to avoid duplicate code.

@lgarrison lgarrison left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SBouchard01! This looks good to me, just some minor nits below.

Also, any reason we need a __init__.py file? Previously we had been using namespace packages. This is probably a harmless addition, but just confirming it was intentional.

Comment thread abacusnbody/hod/menv.py

DEFAULT_BATCH_SIZE = 10**5

logger = logging.getLogger(__file__)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should use __name__ instead of __file__?

DEFAULTS = {}
DEFAULTS['path2config'] = 'config/abacus_hod.yaml'

logger = logging.getLogger(__file__)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@lgarrison

Copy link
Copy Markdown
Member

I think @SandyYuan can answer the questions about the different magic values and the ntarget maximum?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants