skypy icon indicating copy to clipboard operation
skypy copied to clipboard

ENH: galaxy-quenching model

Open Lucia-Fonseca opened this issue 3 years ago • 8 comments

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

  • [x] Follow the Contributor Guidelines
  • [x] Write unit tests
  • [x] Write documentation strings
  • [x] Assign someone from your working team to review this pull request
  • [x] Assign someone from the infrastructure team to review this pull request

Lucia-Fonseca avatar Feb 17 '22 16:02 Lucia-Fonseca

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

Lucia-Fonseca avatar Jun 15 '22 16:06 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 avatar Jul 05 '22 15:07 Lucia-Fonseca

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

Lucia-Fonseca avatar Aug 19 '22 09:08 Lucia-Fonseca

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

itrharrison avatar Aug 19 '22 11:08 itrharrison

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 ?

Lucia-Fonseca avatar Aug 19 '22 15:08 Lucia-Fonseca

@Lucia-Fonseca see #572 :-)

itrharrison avatar Aug 25 '22 10:08 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 😃

itrharrison avatar Nov 22 '22 10:11 itrharrison

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

Lucia-Fonseca avatar Jan 16 '23 16:01 Lucia-Fonseca