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

Update to CCLv3 #97

Merged
merged 9 commits into from
Oct 17, 2023
Merged

Update to CCLv3 #97

merged 9 commits into from
Oct 17, 2023

Conversation

carlosggarcia
Copy link
Contributor

This pull request updates the code to the new API of CCLv3.

I had to slightly reduce the accuracy threshold of some tests since different versions of CCL, NaMaster, etc might have change different interpolation and integration schemes.

@mattkwiecien
Copy link
Contributor

Ran black and flake8 and pushed so CI could continue.

@carlosggarcia carlosggarcia changed the title Updated to CCLv3. ccl tracer test need improving Update to CCLv3 Oct 16, 2023
@carlosggarcia carlosggarcia marked this pull request as ready for review October 16, 2023 22:43
@@ -84,6 +84,8 @@ jobs:
run: |
export MAMBA_NO_BANNER=1
mamba env update --file ${{ env.CONDA_ENV }}
# GitHub complains without this
mamba install coverage
Copy link
Collaborator

@felipeaoli felipeaoli Oct 17, 2023

Choose a reason for hiding this comment

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

@mattkwiecien , I do not think this is a big issue, but can you check this just in case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Messaged in slack, this is because the environment name has changed. We should either change environment name everywhere or revert in enrivonment.yml

environment.yml Outdated
@@ -13,7 +13,7 @@ dependencies:
- numpy
- scipy
- pyyaml
- pyccl>=2.5.0,<2.8.0
- pyccl>=2.3.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

@carlosggarcia , I guess we should fix this dependence at 3.0.0, since we don't expect this to work for pyccl <=2.7.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, that was my intention. my bad

hmf = ccl.halos.MassFuncTinker08(cosmo, mass_def=mass_def)
hbf = ccl.halos.HaloBiasTinker10(cosmo, mass_def=mass_def)
mass_def = ccl.halos.MassDef200m
hmf = ccl.halos.MassFuncTinker08(mass_def=mass_def)
Copy link
Collaborator

@felipeaoli felipeaoli Oct 17, 2023

Choose a reason for hiding this comment

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

Not sure, why we use Tinker08 as default instead of Tinker10. I am using Tinker10 for ng. If there is no obvious reason to use Tinker08, we can keep this (so we don't break the current benchmarks) and I open an issue for keeping track of this (ask to Ryo and Yue)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea. I would leave it like this for this PR to avoid mixing changes

hb = ccl.halos.HaloBiasTinker10(cosmo, mass_def=md)
hmc = ccl.halos.HMCalculator(cosmo, mf, hb, md)
md = ccl.halos.MassDef200m
mf = ccl.halos.MassFuncTinker08(mass_def=md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not Tinker10?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above

tjpcov/covariance_cluster_mass.py Show resolved Hide resolved
@felipeaoli
Copy link
Collaborator

Thanks, @carlosggarcia, everything seems good! I am asking @mattkwiecien to check specific lines

@mattkwiecien mattkwiecien self-requested a review October 17, 2023 14:20
environment.yml Outdated
@@ -1,4 +1,4 @@
name: tjpcov
name: tjpcov_ccl3
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to change this back to tjpcov once you're done testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will actually cause the CI to fail:

activate-environment: tjpcov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved

# elif "wl" in tr:
# assert isinstance(ccltr, ccl.WeakLensingTracer)
# elif "cv" in tr:
# assert isinstance(ccltr, ccl.CMBLensingTracer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we create an issue for this so we don't lose track of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now solved

@@ -84,6 +84,8 @@ jobs:
run: |
export MAMBA_NO_BANNER=1
mamba env update --file ${{ env.CONDA_ENV }}
# GitHub complains without this
mamba install coverage
Copy link
Contributor

Choose a reason for hiding this comment

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

Messaged in slack, this is because the environment name has changed. We should either change environment name everywhere or revert in enrivonment.yml

tjpcov/covariance_cluster_mass.py Show resolved Hide resolved
@mattkwiecien
Copy link
Contributor

LGTM!

@carlosggarcia carlosggarcia merged commit f99d399 into master Oct 17, 2023
12 checks passed
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.

3 participants