ENH: galaxy-quenching model

[Lucia-Fonseca]

Description

This PR adds a feature where we can obtain the Schechter parameters for active galaxies (centrals and satellites) and passive galaxies (mass- and satellite-quenched) based on equation (15) in de la Bella et al. 2021. Merging this PR closes #513 .

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

[philipp128]

For the moment I have mainly comments on naming convention. I found it first hard to figure out what is calculated especially cause of the names in the paper. At the end it does not make a big difference but it was confusing to me.

[rrjbca]

I would suggest refactoring as follows:

• mstar is not used to calculate any of the phi or alpha values and is returned unchanged in all four populations. I would remove mstar from the arguments and return values.
• Similarly, alpha is not used to calculate any of the phi values and is only modified from the input value for one population. That function is trivial (1 + alpha), does not depend on phi, fsatellite or fenvironment, and is a result of a very specific modelling assumption (quenching rate proportional to mass). I would at least move the alpha calculation to a separate function, and probably suggest removing it entirely.
• This leaves just the calculation of the phi values. The active_parameters tuple can be replaced with just phi since there is no longer any dependence on mstar or alpha. I would also split into four functions: phi_centrals, phi_satellites, phi_mass_quenched and phi_satellite_quenched. This makes it much clearer from the name what each function returns and saves the user having to check the dictionary keys in the documentation when calling it. Individual functions can also be used more easily in situations where the user doesn't want to model all four populations.

[Lucia-Fonseca]

Addressed @rrjbca 's comments. Ready for a second review. Thanks

[Lucia-Fonseca]

@rrjbca do I need to make hypothesis a new dependency of skypy to run the tests?
Also I'm a bit confused about the merge conflict.
Thanks for your help.

[Lucia-Fonseca]

Hi @itrharrison I can see your last commits but I do not understand why they should be part of this PR?

[itrharrison]

@Lucia-Fonseca they are fixes to failing codestyle tests which had appeared because (I think) a newer version of flake8 being more strict about whitespace next to keywords like assert.

They are indeed nothing to do with this PR but I fixed them along with the merge conflict so that the tests ran okay. Without the tests running it wasn't clear to me or not if the stuff with hypothesis was going to work.

The PR looks good to me, but I don't know if you want to wait for explicit approval from @rrjbca and @philipp128 from the changes they requested?

[Lucia-Fonseca]

Yes, I'd wait for the rest of their approvals.
Nonetheless, I think if a new flake8 version is making some tests or code fail but not this one on this PR, a new independent issue should be open, along with a different PR. Unless I am mistaken, @rrjbca ?

[itrharrison]

@Lucia-Fonseca see #572 :-)

[philipp128]

With the newest changes, I am happy with everything.

[itrharrison]

@Lucia-Fonseca I think the only thing missing to fulfill @rrjbca's review requests is adding corner cases for the schechter_smf_phi_mass_quenched function. I think these tests do that by the letter of the law, at least 😝

assert phi0 == 0.5 * schechter_smf_phi_mass_quenched(phi0, phi0)
assert 0.0 == schechter_smf_phi_mass_quenched(phi0, -phi0)
assert phi0 == 1. * schechter_smf_phi_mass_quenched(phi0, 0.0)

If you do these I will be happy to dismiss Richard's review if he doesn't respond soon himself 😃

[Lucia-Fonseca]

@itrharrison I just submitted these changes.

@Lucia-Fonseca I think the only thing missing to fulfill @rrjbca's review requests is adding corner cases for the schechter_smf_phi_mass_quenched function. I think these tests do that by the letter of the law, at least 😝

assert phi0 == 0.5 * schechter_smf_phi_mass_quenched(phi0, phi0)
assert 0.0 == schechter_smf_phi_mass_quenched(phi0, -phi0)
assert phi0 == 1. * schechter_smf_phi_mass_quenched(phi0, 0.0)

If you do these I will be happy to dismiss Richard's review if he doesn't respond soon himself 😃