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

Implement mass-concentrations from Ishiyama et al. (c500) #895

Merged
merged 34 commits into from
Jan 13, 2022
Merged

Conversation

nikfilippas
Copy link
Contributor

@nikfilippas nikfilippas commented Jul 8, 2021

Addresses #892 .

Checklist:

  • mass-concentration relation
  • M500 mass definition
  • unit tests
  • benchmarks

@nikfilippas nikfilippas linked an issue Jul 8, 2021 that may be closed by this pull request
@coveralls
Copy link

coveralls commented Jul 8, 2021

Pull Request Test Coverage Report for Build 1689806813

  • 127 of 128 (99.22%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 97.047%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyccl/halos/concentration.py 124 125 99.2%
Totals Coverage Status
Change from base Build 1532031833: 0.2%
Covered Lines: 4403
Relevant Lines: 4537

💛 - Coveralls

@nikfilippas nikfilippas changed the title Implement c500 etc. Implement mass-concentrations from Ishiyama et al. (c500) Jul 14, 2021
@c-d-leonard
Copy link
Collaborator

Is this ready for review? @nikfilippas

@nikfilippas
Copy link
Contributor Author

nikfilippas commented Oct 18, 2021

@c-d-leonard not yet! The science is all there but the computation is very slow. Once I find some time I want to speed it up by ~1 order of magnitude using a shortcut in CCL. I'll ask for someone to review this when it's all done.

Copy link
Collaborator

@damonge damonge left a comment

Choose a reason for hiding this comment

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

Good stuff @nikfilippas !
See below a few minor comments. I'll check timing on my end while you address these

pyccl/halos/massdef.py Outdated Show resolved Hide resolved
pyccl/halos/concentration.py Show resolved Hide resolved
pyccl/halos/concentration.py Show resolved Hide resolved
pyccl/halos/concentration.py Outdated Show resolved Hide resolved
pyccl/halos/concentration.py Outdated Show resolved Hide resolved
benchmarks/test_concentration_Ishiyama21.py Show resolved Hide resolved
@damonge
Copy link
Collaborator

damonge commented Jan 12, 2022

Just a quick note: I just checked and I don't think the computational bottleneck comes from the calculation of dlnsigma/dlnR. If anything it may be dominated by the root finding, but that it's quite fast in itself, so I don't think we should worry.
In summary: ready to go as soon as the previous comments are addressed.

@nikfilippas
Copy link
Contributor Author

You are right, the bottleneck is the computation of sigmaM, in case these haven't already been computed. Then, root finding also takes time but I can't think of any other way. I profiled this one and Duffy08, and this one is an order of magnitude slower. Have a look at the attached images.
Ishiyama21
Screenshot from 2022-01-12 18-27-20
Duffy08
Screenshot from 2022-01-12 18-28-22

@nikfilippas
Copy link
Contributor Author

@damonge ready to be looked at again.

@damonge
Copy link
Collaborator

damonge commented Jan 12, 2022

OK, you'll have to teach me how to generate those timing diagrams ;-)

Just one comment above, and we can merge as soon as that's done.

Copy link
Collaborator

@damonge damonge left a comment

Choose a reason for hiding this comment

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

LGTM

@nikfilippas nikfilippas merged commit a37cad6 into master Jan 13, 2022
@nikfilippas nikfilippas deleted the c500 branch January 13, 2022 10:16
@nikfilippas nikfilippas mentioned this pull request Apr 4, 2023
27 tasks
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.

c500 mass-concentration relation
4 participants