ENH: Subhalo abundance (SHAM) code #614
Conversation
Fox-Davidson
added
documentation
There was a problem hiding this comment.
Looks good!
A few comments on the code, mostly related to removing the internal yaml manipulation.
You should also make sure that documentation exists and compiles properly.
The test suite is impressively comprehensive, well done!
It looks like the halos module which you are merged into is failing its tests due to the classy dependency. I will look into this.
I have not explicitly reviewed the science here; this process is happening independently through the manuscript draft. The extensive test suite should mean that any calculations are performed as intended.
skypy/halos/sham.py
Outdated
| function += ' m_max: $m_max\n sky_area: $sky_area\n' | ||
|
|
||
| # Make one large string | ||
| yaml_lines = line1 + line2 + line3 + line4 + line5 + line6 + line7 + line8 + line9 + line10 |
There was a problem hiding this comment.
It would be better if we could avoid these internal yaml manipulations and replace them with usage of the input yaml to the skypy pipeline, and/or calls to the skypy functions directly in python where necessary.
There was a problem hiding this comment.
See also the argparse module which would be used to do any reading and manipulation necessary, example here:
skypy/skypy/pipeline/scripts/skypy.py
Line 12 in ee05b5a
| gal_type_fin : (nm, ) array_like | ||
| List of assigned galaxies types with the tags 1,2,3,4 for | ||
| red centrals, red satellites, blue centrals, blue satellites | ||
| print_out : Boolean |
There was a problem hiding this comment.
The skypy pipeline call includes a "verbose" flag which could be used instead of this print_out keyword.
There was a problem hiding this comment.
Thanks for the review. I am struggling to work out how to interact with the verbose keyword as I can't see that it is used anywhere in the galaxies module as an example. From the documentation, this would be applied to the code as a command line argument and I wouldn't expect this code to be used as such due to the dictionary output. Any help would be appreciated.
skypy/halos/sham.py
Outdated
| # Get a catalogue for each population | ||
|
|
||
| # Names for created files, randomly generate tag | ||
| rand_tag = int((10**8)*np.random.rand(1)) |
There was a problem hiding this comment.
Can this be done more reliably / reproducibly? If it is for differentiating files when multiprocessing, a processor rank could be used instead. Alternatively a time stamp tag could be used too.
There was a problem hiding this comment.
Possibly removing all of the internal yaml file writing / reading will abrogate the need for this.
| List of ID values for halos | ||
| z_fin: (nh, ) array_like | ||
| List of redshifts for halos | ||
| gal_type_fin: (nh, ) array_like |
There was a problem hiding this comment.
Could this labels be more informative? i.e be strings like 'red_central' rather than integers.
skypy/halos/tests/test_sham.py
Outdated
| min_mass = 10**(7.5) | ||
| max_mass = 10**(14) | ||
|
|
||
| file_test = '/mnt/c/Users/user/Documents/skypy_dev/skypy/skypy/halos/test_gal.yaml' |
There was a problem hiding this comment.
This needs to be a relative not absolute file path.
skypy/halos/tests/test_sham.py
Outdated
|
|
||
| # Open file and check structure | ||
| # Create expected lines | ||
| line1 = 'm_star: !numpy.power [10, 10.58]\n' |
There was a problem hiding this comment.
See above comments on using the skypy pipeline machinery rather than internal yaml manipulation.
|
|
||
| @pytest.mark.skipif(not HAS_COLOSSUS, reason='test requires colossus') | ||
| @pytest.mark.flaky | ||
| def test_assignment(): |
There was a problem hiding this comment.
Can you please add comments (just short ones) describing what each test is for right underneath the def?
| @@ -57,15 +57,15 @@ def test_growth(): | |||
| Dzprime = growth_function_derivative(redshift, cosmology_flat) | |||
|
|
|||
| # Test growth factor | |||
| assert redshift.shape == fz.shape,\ | |||
| assert redshift.shape == fz.shape, \ | |||
There was a problem hiding this comment.
We would not want to touch the power spectrum module here. Not sure if this has been done by some auto tool?
There was a problem hiding this comment.
...okay, this appears to be some stricter codestyle which should be fixed by #630
Description
This PR adds code to compute the subhalo abundance matching of internally generated halo/subhalo and galaxy catalogues based on a monotonic relation between their masses and the quenching model proposed in Peng et al 2010. Galaxy parameters given as examples are from the Schechter functions found in Birrer et al 2015.
Checklist