-
Notifications
You must be signed in to change notification settings - Fork 63
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
ISW #861
Conversation
Pull Request Test Coverage Report for Build 919220562
💛 - Coveralls |
@damonge is this ready for review? missing benchmarks? |
Ready for review @c-d-leonard |
I will either review this or find someone to review it tomorrow. |
There is no rush. FYI, I couldn't find a proper implementation of this in CLASS (it'd be mixed up with a bunch of other low-redshift effects), so I implemented my own alternative version in python for a benchmark... Open to criticism on that front |
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.
This looks good to me. My only hesitation is the benchmark like you said. I think if we can it would be good to at least have someone else code it up just in case you made the same silly error twice. I'm happy to do it - it looks like it should be pretty straightforward? Or we can see if anyone else is keen or has their own code lying around. I'll check on the channel.
Also when I run the tests locally I'm getting a failure on test_nonlin_camb_power to do with the keyword 'mead2020'. I updated my camb but still there - are you seeing that? |
Nope, but it seems to run fine on gha |
@pennalima Do I remember right that you said numcosmo can calculate the ISW power spectrum for a benchmark here? What we're looking for is equation 6 in 1710.03238. |
@c-d-leonard @pennalima since this is taking a while, how about merging this (if the code looks ok) and adding "add external ISW benchmark" to #832 ? |
Hi @damonge and @c-d-leonard , I am working to complete this task by the end of the week. |
@pennalima this is just a conversion factor. If the TCMB is included it means you're considering the absolute fluctuation in the CMB temperature (Delta T), otherwise you're correlating with the relative fluctuation Delta T/T. Since TCMB is a constant this only changes the overall amplitude of the result (and I think correlating with the absolute fluctuation makes more sense) |
OK, merging, and it seems the ISW tracer has been added to the list of "improvable" benchmarks. |
Implements an ISW tracer.