LSSTDESC / CCL

DESC Core Cosmology Library: cosmology routines with validated numerical accuracy
BSD 3-Clause "New" or "Revised" License
145 stars 68 forks source link

IA profile v3 #1074

Closed damonge closed 1 year ago

damonge commented 1 year ago

Ports #922 (by @chrgeorgiou) directly to v3 API

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4927972783


Changes Missing Coverage Covered Lines Changed/Added Lines %
pyccl/halos/profiles/ia.py 148 156 94.87%
<!-- Total: 155 163 95.09% -->
Files with Coverage Reduction New Missed Lines %
pyccl/base/deprecations.py 2 83.84%
<!-- Total: 2 -->
Totals Coverage Status
Change from base Build 4927900328: -0.03%
Covered Lines: 5996
Relevant Lines: 6173

💛 - Coveralls
nikfilippas commented 1 year ago

I'll chime in before this gets merged just to point out that having flags indicating the NC profiles is probably a bad idea. OOP works with inheritance, so it would be best to define an HOD superclass and then use composition, instead of reinstating flags for everything. Having flags like that clutters the code, and makes it error prone. https://stackoverflow.com/questions/41001932/oo-design-inheritance-vs-type-enum-variable For our needs, composition is a simpler and better solution to the issue we have (allowing an HOD subclass to either represent NC or not represent NC).

It also makes no physical sense to define a NC HOD profile, and then be able to change it to non-NC. A profile is, or is not, NC.

Also, all module names in Python are lowercase by convention. https://peps.python.org/pep-0008/#package-and-module-names

chrgeorgiou commented 1 year ago

Going through the code now, will post my comments here. To start, I see in the tests that the halo model is now called by cM = ccl.halos.ConcentrationDuffy08(mass_def="200m") and ccl.halos.SatelliteShearHOD(concentration=cM) but ccl.halos.SatelliteShearHOD(cM) throws an error. Is there no way to initialize the class now without keyword arguments?

damonge commented 1 year ago

@chrgeorgiou yep, indeed. In v3 we introduced a star operator to force users to pass some keyword-only arguments (this is to prevent some functions doing the wrong thing silently only because users passed arguments in the wrong order - we realised there were many instances where this could happen).

Your IA profile is the first fully v3 piece of code!