astropy Comparison Notebook #1
Conversation
|
This isn't rendering fully for me. One possibility is to make this run automatically and render in the docs in this PR for easier review. I'm happy to set this up, I have it running for gwpopulation the directory structure is just a little different here. |
|
It's now rendering for me, maybe I just had too many tabs open in chrome before. The comparisons all look great and all of my comments are aesthetic, so feel free to take with a pinch of salt.
|
|
Yes, some were linear scaled because the difference was negative! I guess you're right that the sign probably doesn't matter much, except for previous users of the A&K approximant so they know when it was an over-vs-under estimate of astropy. But that seems like a very small number of people, as well as a pretty minor detail, so I changed both panels on these plots to be log scaled by default, and the differences to be absolute. |
|
This is looking good, a minor comments:
|
|
I think I addressed everything with my most recent push - I had pushed before finishing completely. I just haven't integrated to the docs yet. Let me know if there's anything else I should change! |
|
Great! and sorry about the premature comments. The new text is great, I think this is pretty ready to go.
Something that isn't worth commenting on in this notebook, but I don't think I mentioned it previously. I realised we can get more things, e.g., absorption distance by generalizing (3) a little more to add an arbitrary extra power of (1 + z) (see https://wcosmo.readthedocs.io/en/latest/api/wcosmo.taylor.analytic_integral.html.) |
|
Oops, thank you for catching these! Hopefully should be fixed now and in the docs. I also added it to I noticed that the index of the examples page has all 3 section headers (see screenshot below) but the section navigation just has the title of the notebook. I'm assuming we don't want this, so in a future commit I'll demote the other headers to subsections and hopefully that will fix this. |
|
As far as the extra powers of 1+z in the integral giving us more flexibility, that's very cool! I wonder if we can add a short example using different powers of 1+z directly to that page in the docs? I think I understand how to do that (I always see examples in very long docstrings when I look at scipy source code) so I can give it a shot. |
I'm not sure there is a good way, the docs can be built locally, but that isn't always enough.
Yes, we should make the other headings subheadings. |
|
I managed to get around the |
wcosmo/taylor.py
Outdated
|
|
||
| .. math:: | ||
|
|
||
| t_L = t_H\int_0^z \frac{dz'}{(1+z)E(z')} |
There was a problem hiding this comment.
These need to be double braced to avoid the autodoc changes, e.g., https://github.com/ColmTalbot/wcosmo/pull/1/files#diff-66f9615ec461802e638b304f84d667e12c1c3467da818dd87356eeffca9b5f45R216.
There was a problem hiding this comment.
This is great and pretty much ready to go!
I'm going to leave notebook comments here, rather than inline comments:
- I think (1) and (2) are inconsistent, is one of the integrands incorrect? It looks like the factors of a differ between the two.
- We should define
$x$ for (3). - Though the two methodologies are the same in the limit $w=-1$...
-
# In 4: has an unnecessary comment, same in some other cells
Co-authored-by: Colm Talbot <[email protected]>
- remove unnecessary comments - fix error in DL definition - define all variables
Some comparisons with astropy. Will be changed to use the code in this repo