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

LPT NL PT #1097

Merged
merged 11 commits into from
Jul 12, 2023
Merged

LPT NL PT #1097

merged 11 commits into from
Jul 12, 2023

Conversation

anicola
Copy link
Contributor

@anicola anicola commented Jul 8, 2023

This PR implements LPT in the nl_pt modules. This mostly follows the implementation for EPT, but I have included a bunch of additional terms for b_nabla2, and also the template function is a bit different due to the presence of the 1 and delta_L terms. I can change that though if we want full consistency.

@anicola anicola requested a review from damonge July 8, 2023 16:11
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.

Thanks a lot @anicola ! Just a few comments. In the meantime I'll start looking at the tests.

pyccl/nl_pt/lpt.py Outdated Show resolved Hide resolved
pyccl/nl_pt/lpt.py Outdated Show resolved Hide resolved
pyccl/nl_pt/lpt.py Show resolved Hide resolved
pyccl/nl_pt/lpt.py Outdated Show resolved Hide resolved
pyccl/nl_pt/lpt.py Show resolved Hide resolved
@damonge
Copy link
Collaborator

damonge commented Jul 11, 2023

FYI, I removed the heft_aemulus import, since I think it was a leftover and I wanted to see if tests pass. Bring it back if it was intentional!

@damonge damonge mentioned this pull request Jul 11, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 5519362457

  • 187 of 205 (91.22%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 97.35%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyccl/nl_pt/lpt.py 186 204 91.18%
Totals Coverage Status
Change from base Build 5513816721: -0.2%
Covered Lines: 5511
Relevant Lines: 5661

💛 - Coveralls

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!



@pytest.mark.parametrize('kind', ['m:m', 'm:b2', 'm:b3nl', 'm:bs',
'b1:b3nl', 'b3nl:b3nl', 'b3nl:bk2'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: you only added these because testing all combinations takes a bit too long, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's OK, btw, just wanted to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's the thing driving the coverage. The problem is the 1, delta_L split in LPT. Because of that you can't isolate the bias terms as you can in EPT. I think these are the only ones that work.

@damonge damonge merged commit 6c91ee9 into master Jul 12, 2023
3 of 4 checks passed
@damonge damonge deleted the nl_bias_dev branch July 12, 2023 13:23
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.

None yet

3 participants