Skip to content

Conversation

sutieng
Copy link
Contributor

@sutieng sutieng commented Feb 9, 2021

Description

We add a new feature to sample halos from colossus halo mass function.

Checklist

  • Follow the Contributor Guidelines
  • Write unit tests
  • Write documentation strings
  • Assign someone from your working team to review this pull request
  • Assign someone from the infrastructure team to review this pull request

@sutieng sutieng requested a review from a team as a code owner February 9, 2021 07:02
@rrjbca rrjbca requested review from a team and rrjbca February 9, 2021 12:13
@rrjbca rrjbca added module: halos new feature New feature, such as a new model labels Feb 9, 2021
Copy link
Contributor

@rrjbca rrjbca left a comment

Choose a reason for hiding this comment

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

Looks good so far. A few suggestions to generalise the function and make it more useful. I would also suggest:

  • This file should be in the halos directory and renamed. I suggest git mv skypy/clusters/sampler.py skypy/halos/_colossus.py.
  • Update the docstring and docs/halos/index.rst to ensure the new function appears in the documentation
  • Write some unit tests. Perhaps look at the tests for schechter_lf_magnitude as that is quite a similar function

If you need help with any of this just ask

Copy link
Member

@Lucia-Fonseca Lucia-Fonseca left a comment

Choose a reason for hiding this comment

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

  • Agreed with @rrjbca 's comments.
  • Please, check the flake8 style error messages.
  • Can you add the equation number in Dispali+16 as a reference?

Copy link
Contributor

@rrjbca rrjbca left a comment

Choose a reason for hiding this comment

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

Would it be reasonable to refer to the objects sampled by this function as "halos" rather than "clusters" throughout the docstrings here? i.e. are "clusters" simply "high-mass halos" in this context?

@rrjbca
Copy link
Contributor

rrjbca commented Feb 23, 2021

@sutieng could you please add a "Description" for this pull request? We also need to write some unit tests and integrate the function docstring into the documentation, are you able to do these or would you like one of us to help?

sutieng and others added 2 commits February 24, 2021 15:31
Co-authored-by: Lucia F. de la Bella <[email protected]>
Co-authored-by: Lucia F. de la Bella <[email protected]>
@sutieng
Copy link
Contributor Author

sutieng commented Feb 24, 2021

Would it be reasonable to refer to the objects sampled by this function as "halos" rather than "clusters" throughout the docstrings here? i.e. are "clusters" simply "high-mass halos" in this context?
Yes, I agree. Since here we are only considering DM component, so cluster can be referred to high mass halos. I will make changes of the docstrings.

@sutieng
Copy link
Contributor Author

sutieng commented Feb 24, 2021

@sutieng could you please add a "Description" for this pull request? We also need to write some unit tests and integrate the function docstring into the documentation, are you able to do these or would you like one of us to help?
I don't have experience on writing unit tests. It would be great if you could help and let me know what items/ tests are needed to be included in unit tests.

Copy link
Contributor

@rrjbca rrjbca left a comment

Choose a reason for hiding this comment

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

Thanks for writing the tests and documentation they look great. Just a few suggestions:

  • Move imports and skip unit test to avoid an ImportError if colossus is not installed
  • Make sure to install colossus on ReadTheDocs
  • Also test the maximum value and the shape of the samples

sutieng and others added 2 commits March 5, 2021 15:24
Co-authored-by: Richard R <[email protected]>
Copy link
Contributor

@rrjbca rrjbca left a comment

Choose a reason for hiding this comment

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

Excellent, the tests are running and the documentation is building now. Just a couple of small final changes:

  • If you look at the documentation here you can see some parts have not rendered correctly, mostly due to whitespace and units. See suggestions for how to improve this.
  • Since this is your first contribution could you add your details to .zenodo.json as a creator

@rrjbca rrjbca force-pushed the cluster_sampler_v01 branch from e45563f to c79ada9 Compare March 9, 2021 11:48
Copy link
Contributor

@rrjbca rrjbca left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for working on this @sutieng

@rrjbca rrjbca requested a review from Lucia-Fonseca March 9, 2021 11:51
Copy link
Member

@Lucia-Fonseca Lucia-Fonseca left a comment

Choose a reason for hiding this comment

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

This works perfectly.
Great job @sutieng!

@rrjbca rrjbca changed the title Add a mass sampler for the colossus halo mass function ENH: Add a mass sampler for the colossus halo mass function Mar 9, 2021
@rrjbca rrjbca merged commit 1aa09dc into skypyproject:module/halos Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: halos new feature New feature, such as a new model

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants