Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IA profile v3 #1074

Merged
merged 48 commits into from
May 9, 2023
Merged

IA profile v3 #1074

merged 48 commits into from
May 9, 2023

Conversation

damonge
Copy link
Collaborator

@damonge damonge commented Apr 19, 2023

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

nikfilippas and others added 30 commits March 20, 2023 16:43
* Added new baryons module that will deprecate old BCM
* Tracers in V3
* Background, boltzmann and cls in v3
* Covariances in V3
* Correlations in v3
* done

* Make `T_ncdm` a cosmological parameter + (v3) (#1049)

* first commit

* rename TNCDM to T_ncdm but preserve API

* T_CMB directly into cosmo struct

* Omega_g in ccl_parameters_create; no split between C/Python

* physical_constants.T_CMB doesn't mutate anymore!

* force mutate T_CMB in benchmarks

* define defaults on instantiation just in case constants mutate

* A_s & sigma8: don't play with numbers

* simplify

* remove leftover mallocs

* temporarily preserve API for CCLv3

* Unfreeze option for `physical_constants` (#1050)

* first commit

* update rtd

* addressed comments 1

* Remove `T_CMB` and `T_ncdm` as constants (reloaded) (#1058)

* first commit

* OmNuh2 fix

* addressed comments

* keep only massive

* first step

* temp commit

* debug neutrinos & deprecate Omnuh2

* ccl_omega_x & ccl_Omeganuh2 consistency

---------

Co-authored-by: David Alonso <[email protected]>

* RTD: Removed The Docs

---------

Co-authored-by: David Alonso <[email protected]>

* flaked

---------

Co-authored-by: Nick Koukoufilippas <[email protected]>
* tk3d and pk2d in v3

---------

Co-authored-by: Nick Koukoufilippas <[email protected]>
* New PT bias framework
* ported changes in halos/profiles

* changes from star in halos/ (no tests)

* refactor concentration, mass_function, halo_bias; improvements in HMCalculator

* HaloProfile subclasses & HaloProfile.normprof attribute

* simplify imports

* remove HaloProfile.name

* cleaned up pk_Npt

* add extrap_pk argument

* FFTLogParams class instead of dictionary in HaloProfile

* simplified code

* removed mass_def from HMIngredients; comprehensive code review 1

* re-implementation, bugfixes, tests

* c_m_relation >> concentration; code improvements; A_SPLINE_MAX in Python

* HMIngredients initializers - no repeated code

* rogue file

* patch for new changes

* updated Build Status badge website

* fix websites

* added a DOI badge & links

* added nonbreaking space

* removed extra space

* FancyRepr out of CCLObject to simplify

* update tests to match changes

* coverage

* simple test for tkkssc

* more tests for tkkssc

* addressed comments 1

* addressed comments 2: mass_def defaults are name strings etc.

* deprecate Gaussian & PowerLaw profiles

* bugfix in master: sometimes mass_def_strict=False fails unexpectedly

* typo

* addressed comments

* New feature: Arbitrary function for profile normalization

* gain efficiency

* comments + deprecate k_min from HMCalculator

* renamed initialize_from_input --> create_instance

* brought in CCLNamedClass from docs_v3

* brought in from docs_v3: directly callable HMIngredients

* prep for Davids input

* renamed HMCalculator --> HaloModel

* fix typo

* brought in from docs_v3: initialize mass_def from any string (e.g. 400c)

* Revert "renamed HMCalculator --> HaloModel"

This reverts commit 43591ce.

* counterterms func inside of 4pt func

* replace deprecated abstractproperty

* abstract linked methods for HaloProfile, MassFunc, HaloBias, Concentration

* fully working implementation

* renamed lM --> log10M etc.

* minor cosmetic fix

* alternative way to include linked abstract methods and declare the template methods

* reorder

* even simpler

* bring back eq_attrs

* r -> r_t

* Additional halos features (#1068)

* first commit

* enclose classmethods in the class

* brought in minor improvements to halos

* coverage
* ported changes in halos/profiles

* changes from star in halos/ (no tests)

* refactor concentration, mass_function, halo_bias; improvements in HMCalculator

* HaloProfile subclasses & HaloProfile.normprof attribute

* simplify imports

* remove HaloProfile.name

* cleaned up pk_Npt

* add extrap_pk argument

* FFTLogParams class instead of dictionary in HaloProfile

* simplified code

* removed mass_def from HMIngredients; comprehensive code review 1

* re-implementation, bugfixes, tests

* c_m_relation >> concentration; code improvements; A_SPLINE_MAX in Python

* HMIngredients initializers - no repeated code

* rogue file

* patch for new changes

* updated Build Status badge website

* fix websites

* added a DOI badge & links

* added nonbreaking space

* removed extra space

* FancyRepr out of CCLObject to simplify

* update tests to match changes

* coverage

* simple test for tkkssc

* more tests for tkkssc

* addressed comments 1

* addressed comments 2: mass_def defaults are name strings etc.

* deprecate Gaussian & PowerLaw profiles

* remove all warnings in testing

* use pytest as per docs instead of numpy.testing and pyutils.assert_warns

* bugfix in master: sometimes mass_def_strict=False fails unexpectedly

* typo

* addressed comments

* New feature: Arbitrary function for profile normalization

* gain efficiency

* comments + deprecate k_min from HMCalculator

* renamed initialize_from_input --> create_instance

* brought in CCLNamedClass from docs_v3

* brought in from docs_v3: directly callable HMIngredients

* prep for Davids input

* renamed HMCalculator --> HaloModel

* fix typo

* brought in from docs_v3: initialize mass_def from any string (e.g. 400c)

* Revert "renamed HMCalculator --> HaloModel"

This reverts commit 43591ce.

* comments

* comments
* __eq_attrs__ for nl_pt
* extricated normprof
* first commit

* Einasto mass translator + MassDef concentration API

* universal api no-break

* comments

* MassDef vars
* coverage

* comments
Base automatically changed from v3_wip to master May 3, 2023 08:01
@damonge damonge changed the base branch from master to v3_wip May 3, 2023 10:30
@damonge damonge changed the base branch from v3_wip to master May 3, 2023 10:39
@coveralls
Copy link

coveralls commented May 4, 2023

Pull Request Test Coverage Report for Build 4927972783

  • 155 of 163 (95.09%) changed or added relevant lines in 4 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 97.133%

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

💛 - Coveralls

Copy link
Contributor

@carlosggarcia carlosggarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed with @damonge, I have checked all the non-IA changes and they look good to me.

@nikfilippas
Copy link
Contributor

nikfilippas commented May 4, 2023

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
Copy link
Contributor

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?


def get_normalization(self, cosmo, a, hmc):
"""Returns the normalization of this profile, which is the
mean galaxy number density.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently testing this normalization but this seems to not be doing the right thing. Also, in the _get_prefactor method in my PR the normalization is 1/n_gal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it's n_gal (the galaxy number density).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have run some more tests and this does what it should do, but now the user has to explicitly request the normalization of the profiles (which is the correct thing to do). I also noticed this will be the default option in v3, correct?

@damonge
Copy link
Collaborator Author

damonge commented May 8, 2023

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

@damonge damonge merged commit 0264d66 into master May 9, 2023
@damonge damonge deleted the ia_profile branch May 9, 2023 16:31
@damonge damonge mentioned this pull request May 9, 2023
@nikfilippas nikfilippas restored the ia_profile branch May 10, 2023 11:43
@nikfilippas nikfilippas deleted the ia_profile branch May 10, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants