-
Notifications
You must be signed in to change notification settings - Fork 64
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
Draft implementation of Cov NG terms #869
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start! I think I'd leave the 2,2 part for now until we have a good idea for how to integrate over the angle...
FYI @carlosggarcia this is failing the tests due to some flake8 formatting issues. You can see them if you climb on the Details above for the continuous integration tests. Let me know if you need any help with fixing these. |
@carlosggarcia could we get a status update on this? :) anywhere you need help to move forward? |
@c-d-leonard The update is that there's no update. I was focusing on adding NaMaster to TJPCov and now on getting it work with TXPipe. So the problem has been time, not an actual problem with the implementation! |
Hi @carlosggarcia just checking whether the update is still there's no update? no worries if so :) just doing an inventory before next telecon |
@c-d-leonard I'm afraid it is. Sorry! I'll try get back to this in a few weeks. |
Looks like some progress here :) @carlosggarcia any support you need on this at this point? |
@c-d-leonard, thanks for asking! @damonge is already helping me with the maths. I think that will be enough for now. |
Just wondering how things are going with this at this point @carlosggarcia? No pressure, just want to check how it's going and if you need anything. |
@c-d-leonard same as two months ago. I'll try to give it a push in the next two weeks. |
Finally, this is ready for review! @damonge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @carlosggarcia , this is really great.
See comments below. One overall comment: the new functions in pk_4pt
could be optimized by comparing profiles, and avoiding recomputing some integrals if they've been already computed (you can check out what's being done in the 1h trispectrum). If you can see an easy way of doing that now, go for it, but I'm also happy to just leave a comment and optimise this later on (since this is already a huge PR!).
I have addressed the easy comments. I'll look now how to optimize it. |
back to you @damonge |
Co-authored-by: David Alonso <david.alonso@physics.ox.ac.uk>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments (plus remaining previous conversations)
I think all it's done now. Back to you @damonge. Thanks for reviewing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final comments on the tests. We're almost there!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Feel free to merge if the tests pass!
I have started the implementation of the missing terms in the Cov NG. So far I have implemented the
T^2h_13
term and checked that runs but haven't checked if the result makes sense.I'm following Eq. 3.25 of https://arxiv.org/pdf/1912.08209.pdf