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

Hubble parametrized #31

Merged
merged 5 commits into from
Mar 8, 2023
Merged

Hubble parametrized #31

merged 5 commits into from
Mar 8, 2023

Conversation

nstarman
Copy link
Contributor

@nstarman nstarman commented Mar 2, 2023

I think the StandardCosmology should only be an inheritance of other components. Therefore, we should separate out the Hubble (and later the Tcmb) pieces. This PR makes the Protocol HubbleParametrized which holds the Hubble info.

PR Checklist

  • Give a detailed description of the PR above.
  • Document changes in the CHANGES.rst file. See existing changelog for examples.
  • Add tests, if applicable, to ensure code coverage never decreases.
  • Make sure the docs are up to date, if applicable, particularly the docstrings and RST files in docs folder.
  • Ensure linear history by rebasing, when requested by the maintainer.

@nstarman nstarman added this to the v0.1 milestone Mar 2, 2023
@nstarman nstarman requested a review from ntessore March 2, 2023 20:40
@ntessore
Copy link
Contributor

ntessore commented Mar 2, 2023

Can you explain this change a little more? As I understand it, a Hubble parameter comes with having a scale factor, which comes with knowing what a cosmological redshift is... So in this picture, every FLRW cosmology should be HubbleParametrized intrinsically.

@nstarman
Copy link
Contributor Author

nstarman commented Mar 3, 2023

As in, we should package it into FLRWBackground?

@ntessore
Copy link
Contributor

ntessore commented Mar 3, 2023

I just don't see how it could be separate from the distance functions. The Hubble function is the connection between redshift and physical time, from which you get the comoving distance.

Hm, ok, on the implementation side, I see how a cosmology instance might not expose the H0 and H(z) details, in which case one would have to manually differentiate the comoving distance somehow to get it back ...

Maybe the name sent me in the wrong direction, because it's not parametrised by the Hubble function. Perhaps HubbleParameter as an alternative?

@nstarman
Copy link
Contributor Author

nstarman commented Mar 4, 2023

Maybe the name sent me in the wrong direction, because it's not parametrised by the Hubble function. Perhaps HubbleParameter as an alternative?

That sounds reasonable, or what about HasHubbleParameter?

@nstarman
Copy link
Contributor Author

nstarman commented Mar 4, 2023

Also, then I suppose we would separate out HasTCMB?
I've been trying to decide whether HasTCMB would require support for photons? Technically the CMB background temperature is the temperature of the photons, so if the are no photons.... OTOH often in simple models, e.g. only matter and dark energy we still talk about the background temperature.

@ntessore
Copy link
Contributor

ntessore commented Mar 4, 2023

I think if we want to separate everything, then yes, we should separate truly everything. This is similarly more about the presence of certain methods in the object than about physics, so there probably isn't a contradiction either.

I like the HasX names, but then we probably also want to change all the components (again) to be HasXComponent.

@nstarman
Copy link
Contributor Author

nstarman commented Mar 7, 2023

Ok. Followup PR will separate out the Tcmb and rename the components.

@nstarman
Copy link
Contributor Author

nstarman commented Mar 7, 2023

What are your thoughts on HasTcmb requiring (or not) HasPhotonComponent ?

@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2023

Codecov Report

Merging #31 (3d9f5d5) into main (2adb3c8) will increase coverage by 0.68%.
The diff coverage is 88.46%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #31      +/-   ##
==========================================
+ Coverage   75.91%   76.59%   +0.68%     
==========================================
  Files          12       13       +1     
  Lines         274      282       +8     
==========================================
+ Hits          208      216       +8     
  Misses         66       66              
Impacted Files Coverage Δ
src/cosmology/api/_extras.py 87.50% <87.50%> (ø)
src/cosmology/api/__init__.py 100.00% <100.00%> (ø)
src/cosmology/api/_standard.py 89.47% <100.00%> (+3.36%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nstarman nstarman marked this pull request as draft March 7, 2023 19:47
@nstarman nstarman force-pushed the hubble-parametrized branch 2 times, most recently from 62849f0 to 40099a1 Compare March 7, 2023 21:08
@nstarman nstarman marked this pull request as ready for review March 7, 2023 21:09
@nstarman
Copy link
Contributor Author

nstarman commented Mar 7, 2023

@ntessore, ready for review.

One thing I've considered changing w.r.t astropy is inv_efunc to efunc_inv. I think this makes the relationship to efunc more explicit. What do you think? Also is efunc a good name? What would be a better alternative?

@ntessore
Copy link
Contributor

ntessore commented Mar 7, 2023

What are your thoughts on HasTcmb requiring (or not) HasPhotonComponent ?

I don't see a benefit to requiring it. It might not be exposed by the underlying cosmology object, similar to the point of this PR.

One thing I've considered changing w.r.t astropy is inv_efunc to efunc_inv. I think this makes the relationship to efunc more explicit. What do you think? Also is efunc a good name? What would be a better alternative?

I find both inv_efunc and efunc_inv poor choices because it makes me think that is an inverse function returning a redshift given a value. Also, what's the computational advantage of having it? I can only think of sticking it directly into a numerical quadrature, but if a cosmology implementation wants to do that internally, the method doesn't have to be exposed.

ntessore
ntessore previously approved these changes Mar 7, 2023
Copy link
Contributor

@ntessore ntessore left a comment

Choose a reason for hiding this comment

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

LGTM. I would not have put this into its own submodule, it can just live next to the main cosmology implementation, to save the time for one more import.

@nstarman
Copy link
Contributor Author

nstarman commented Mar 7, 2023

When mypyc has fixed some Issues with runtime-checkable protocols we can compile this whole library into c-wheels. Long term, speed will never be an issue.

@nstarman
Copy link
Contributor Author

nstarman commented Mar 7, 2023

I don't see a benefit to requiring it. It might not be exposed by the underlying cosmology object, similar to the point of this PR.

SGTM

I find both inv_efunc and efunc_inv poor choices because it makes me think that is an inverse function returning a redshift given a value. Also, what's the computational advantage of having it? I can only think of sticking it directly into a numerical quadrature, but if a cosmology implementation wants to do that internally, the method doesn't have to be exposed.

For v0.1 we can remove inv_efunc. Punt the decision down the road a bit.
What about the name efunc?

@ntessore
Copy link
Contributor

ntessore commented Mar 8, 2023

For v0.1 we can remove inv_efunc. Punt the decision down the road a bit. What about the name efunc?

Sounds good. I don't know of any other, more recognisable name for efunc. CCL calls it h_over_h0 but I don't find that better.

@nstarman
Copy link
Contributor Author

nstarman commented Mar 8, 2023

For v0.1 we can remove inv_efunc. Punt the decision down the road a bit. What about the name efunc?

Sounds good. I don't know of any other, more recognisable name for efunc. CCL calls it h_over_h0 but I don't find that better.

We have H for the Hubble Parameter. What about EH?

@ntessore
Copy link
Contributor

ntessore commented Mar 8, 2023

For v0.1 we can remove inv_efunc. Punt the decision down the road a bit. What about the name efunc?

Sounds good. I don't know of any other, more recognisable name for efunc. CCL calls it h_over_h0 but I don't find that better.

We have H for the Hubble Parameter. What about EH?

Those uppercase function names violate PEP 8 and look like constants, so that makes efunc superior in my opinion.

@nstarman
Copy link
Contributor Author

nstarman commented Mar 8, 2023

Those uppercase function names violate PEP 8 and look like constants, so that makes efunc superior in my opinion.

😮‍💨. Yeah, it's a problem with H, but thankfully not H0. My issues with efunc are that it feels silly to name a function func, even more so when it's technically a method.

Signed-off-by: nstarman <[email protected]>
src/cosmology/api/_extras.py Outdated Show resolved Hide resolved
Co-authored-by: Nicolas Tessore <[email protected]>
Signed-off-by: Nathaniel Starkman <[email protected]>
@nstarman nstarman requested a review from ntessore March 8, 2023 16:27
@nstarman
Copy link
Contributor Author

nstarman commented Mar 8, 2023

I'll do a squash commit when it's ready.

@nstarman nstarman merged commit 9666120 into main Mar 8, 2023
@nstarman nstarman deleted the hubble-parametrized branch March 8, 2023 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants