ENH: Add concentration, radius, Delta_Sigma functions to halos module#467
ENH: Add concentration, radius, Delta_Sigma functions to halos module#467sutieng wants to merge 1 commit intoskypyproject:module/halosfrom
Conversation
|
Hi Sutieng, thank you for your pull request. Could you please set up your git username and email correctly for GitHub? There is a description in the GitHub documentation. Afterwards, you will need to fix the existing commits in this PR. The easiest that I can think of is resetting your commits without undoing any of the changes, then simply committing again, which will use the new username and email: $ git reset --soft module/halos
$ git commit
$ git push --force |
sutieng
force-pushed
the
Add_halos_features
branch
from
e579554 to
d60129e
Compare
|
Thanks Nicolas! I've just set up the git email and re-pushed all the commits. |
rrjbca
changed the title
Lucia-Fonseca
left a comment
Lucia-Fonseca
left a comment
There was a problem hiding this comment.
Good job!
Very nice and clear.
A few comments on:
- Units with
h(to check) - Docstrings
- More tests?
| Parameters | ||
| ---------- | ||
| mass : float or array_like | ||
| Spherical overdensity halo mass in units of solar mass/h, corresponding |
There was a problem hiding this comment.
We had a debate about the use of h in units. We finally decided to work in units without h.
Is this relevant for you here?
| Spherical overdensity halo mass in units of solar mass/h, corresponding | ||
| to the mass definition, mdef. | ||
| mdef : str | ||
| Halo mass definition for spherical overdensities used by colossus. |
There was a problem hiding this comment.
Which are the options to choose from? Is it worth adding them here?
| Returns | ||
| ------- | ||
| concentration : float or array_like | ||
| Halo concentration(s); has the same dimensions as mass. |
| r'''Calculate the scale radius and the spherical overdensity radius of halo by assuming | ||
| the NFW model. |
There was a problem hiding this comment.
| r'''Calculate the scale radius and the spherical overdensity radius of halo by assuming | |
| the NFW model. | |
| r'''Halo radii with the NFW model. |
| return c | ||
|
|
||
|
|
||
| def radius(mass, concentration, redshift, mdef, Delta, cosmology, sigma8, ns): |
There was a problem hiding this comment.
Is this function name descriptive enough? what about radii_NFW or anything better??
| mdef : str | ||
| Halo mass definition for spherical overdensities used by colossus. | ||
| radius : float or array_like | ||
| Radius in physical kpc/h. |
There was a problem hiding this comment.
See above re: discussion about units and h.
| def test_colossus_concentration(): | ||
| from skypy.halos._colossus import concentration | ||
| from astropy.cosmology import Planck15 | ||
|
|
There was a problem hiding this comment.
We could add a comment here, briefly explaining what this test is doing.
For example:
# Check concentration array matches mass lengthor similar
| def test_colossus_radius(): | ||
| from skypy.halos._colossus import radius | ||
| from astropy.cosmology import Planck15 | ||
|
|
There was a problem hiding this comment.
Add a short comment explaining.
| def test_colossus_Delta_Sigma(): | ||
| from skypy.halos._colossus import Delta_Sigma | ||
| from astropy.cosmology import Planck15 | ||
|
|
|
|
||
| DeltaSigma = Delta_Sigma(mass, c, redshift, mdef, radius, cosmo, sigma8, ns) | ||
|
|
||
| assert len(DeltaSigma) == len(radius) |
There was a problem hiding this comment.
In general, is there anything else we can test here? Corner cases? Does any of this follow a formula for certain parameters...?
3 participants
Description
This implements several functions to derive the halo concentration, scale radius, the spherical overdensity radius and the excess surface density for halos module.
Checklist