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
add a cached halotools catalogs + HOD cleanup #441
Conversation
@rainwoodman I am ready to merge this now, if you are. |
|
||
# missing required column | ||
del cat.halo_table['halo_id'] | ||
# no galaxies populated into halos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logMmin is very large. I'll add a note
size = hod.csize | ||
|
||
# repopulate (with same seed --> same catalog) | ||
hod.repopulate(seed=42) | ||
hod = halos.repopulate(seed=42) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you like hod.repopulate() better than halos.repopulate()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case then the first ValueError can be eliminated -- without doing populate first there is no hod object for you to repopulate.
The implication is that the compiled model shall be associated with hod, not halos.
assert hod.csize == size | ||
|
||
# repopulate with random seed --> different catalog | ||
hod.repopulate() | ||
hod = halos.repopulate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably give an explicit seed ? It is unlikely but what if random number generator just gave the same number as you used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am actually testing that the seed is set by root and then propagated when it is None − I'll update the assertion
@MPITest([1]) | ||
def test_download_file(comm): | ||
|
||
# download Gadget1P snapshots directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: TPM, not Gadget1P
I noted a few minor glitches in the comments. hod.repopulate() and halo.populate() can the assertion no repopluate before populate. It encourages moving the compiled model to hod -- but I suspect this works against the data model of halotools. We can still mark halo._repopulate() private and only expose it via hod.repopulate(), then comment about the reason for using a delegation . |
Okay, I agree with your repopulate() comment. Now, halos.populate returns a PopulatedHaloCatalog (a light subclass of ArrayCatalog) which has the repopulate() method (which does the repopulation in place). I'll merge this if you are ready. |
Yes looks good!
…On Nov 18, 2017 10:10 AM, "Nick Hand" ***@***.***> wrote:
Okay, I agree with your repopulate() comment. Now, halos.populate returns
a PopulatedHaloCatalog (a light subclass of ArrayCatalog) which has the
repopulate() method (which does the repopulation in place).
I'll merge this if you are ready.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#441 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAIbTMkxnTuBxJUIITTO_LLS0MbN3Ny5ks5s3x19gaJpZM4QWjiz>
.
|
A few additions: