search

skypyproject / skypy

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

ENH: galaxy-quenching model #516

Open Lucia-Fonseca opened 2 years ago

Lucia-Fonseca commented 2 years ago

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

Lucia-Fonseca commented 2 years ago

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

Lucia-Fonseca commented 2 years ago

@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 commented 2 years ago

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

itrharrison commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@Lucia-Fonseca see #572 :-)

itrharrison commented 1 year ago

@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 commented 1 year ago

@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 😃