search

skypyproject / skypy

SkyPy: A package for modelling the Universe.
BSD 3-Clause "New" or "Revised" License
118 stars 38 forks source link

Add velocity dispersion function sampler to skypy #573

Closed Vikramb1 closed 1 year ago

Vikramb1 commented 1 year ago

Description

Add a velocity dispersion function sampler to skypy, which is describes the populations of lenses in sigma_v space. Sigma_v is velocity dispersion of the lens. The equation used for this is found in Choi et al. (2017). The paper uses data of Elliptical galaxies from the Sloan Digital Sky survey. Closes #575.

Checklist

sibirrer commented 1 year ago

@Lucia-Fonseca @itrharrison or others, is this PR in review or let us know if and how we can proceed. Thank you!

philipp128 commented 1 year ago

Thanks @Vikramb1 for the PR. Could you add a description for this PR and link to an open issue which this PR is trying to resolve? If no issue exist, could you open one? This would help to understand what this PR is adding and makes the review process easier.

Vikramb1 commented 1 year ago

Thanks @Vikramb1 for the PR. Could you add a description for this PR and link to an open issue which this PR is trying to resolve? If no issue exist, could you open one? This would help to understand what this PR is adding and makes the review process easier.

Hi Philipp, just done that now. I am a bit new to all this, so let me know if there is anything you want to change or explain better.

philipp128 commented 1 year ago

Thanks @Vikramb1 for the PR. Could you add a description for this PR and link to an open issue which this PR is trying to resolve? If no issue exist, could you open one? This would help to understand what this PR is adding and makes the review process easier.

Hi Philipp, just done that now. I am a bit new to all this, so let me know if there is anything you want to change or explain better.

Thanks, much better! One more thing though, it is always helpful to link to the corresponding issue. We normally do this by adding "Closes #Issue_number". This has the advantage of automatically closing the issue if the PR is merged.

I also approved the contribution such that the tests can run. Note that the flake8 test failed, which is checking for code style. As a first step, can you try to fix this? You can install flake8 on your machine and just run it with flake8 path/to/file. Please check both files, i.e. also the test file. Thanks!

Vikramb1 commented 1 year ago

Thanks @Vikramb1 for the PR. Could you add a description for this PR and link to an open issue which this PR is trying to resolve? If no issue exist, could you open one? This would help to understand what this PR is adding and makes the review process easier.

Hi Philipp, just done that now. I am a bit new to all this, so let me know if there is anything you want to change or explain better.

Thanks, much better! One more thing though, it is always helpful to link to the corresponding issue. We normally do this by adding "Closes #Issue_number". This has the advantage of automatically closing the issue if the PR is merged.

I also approved the contribution such that the tests can run. Note that the flake8 test failed, which is checking for code style. As a first step, can you try to fix this? You can install flake8 on your machine and just run it with flake8 path/to/file. Please check both files, i.e. also the test file. Thanks!

Ah, ok. I added a reference to the issue and I think the code should pass the flake8 test now. Thanks!

Vikramb1 commented 1 year ago

I noticed the coverage test fail, and I think the latest commit should solve that.

Vikramb1 commented 1 year ago

Is there anything else you would like me to do for this pr @philipp128 ?

Vikramb1 commented 1 year ago

From the context I assume that you implement a sampler based on equation (4) of the Choi et al. paper. Please clarify this in the docstring.

I see that the velocity dispersion is simply a Schechter function, which is already implemented in SkyPy. I suggest that you follow the same syntax. I.e. add a module velocity_dispersion.py to the galaxy package and in this module add a function schechter_vdf(). You can find examples in skypy.galaxies.luminosity, skypy.galaxies.redshift and skypy.galaxies.stellar_mass.

We have also already implemented a sampler schechter to sample from the Schechter function in skypy.utils.random. You can use it in the same way as done in the examples mentioned above. In this case you don't need the class PDFSampling and the function approx_cdf_1d().

I added some further comments about naming conventions at the specific lines in the codes.

Hi Philipp - thanks for the feedback. I have committed the changes. Please let me know what you think.