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

A hierarchy of types #48

Merged
merged 12 commits into from
Apr 17, 2023
Merged

A hierarchy of types #48

merged 12 commits into from
Apr 17, 2023

Conversation

nstarman
Copy link
Contributor

Fine grain protocols of attrs and methods.

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 Apr 13, 2023
@nstarman nstarman requested a review from ntessore April 13, 2023 18:33
@nstarman
Copy link
Contributor Author

nstarman commented Apr 13, 2023

@ntessore, I had to change the name of the Hubble and critical density aggregate protocols and I am not thrilled with the new name. Perhaps we should systematize the naming structure a little better:

  1. has an attribute / method -> Has<X>
  2. low-level aggregator, e.g. HasOmegaTot0 & HasOmegaTot -> [Prefix I haven't thought of]TotalDensity[Suffix]
    • the suffix for components would be Component
    • Distance for the distances, like Tcmb ?
    • Nothing for the extras, like Hubble and scale factor?
  3. mid-level aggregator, e.g. DistanceMeasures -> [Whatever makes sense]
  4. high-level aggregator: Cosmology : e.g. Cosmology and StandardCosmology

What would be nice is to have a consistent name with HasHubbleMethods and HasCriticalDensityMethods and HasBackgroundT, which I've put in as placeholder names.
We can keep the Component suffix for the components, but maybe we should add Density to Has<X>Component?
I'm not sure what a good prefix for the low-level aggregator is. Has isn't bad, but it's not great either because then something like the Methods suffix feels necessary, which I don't particularly like.

@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2023

Codecov Report

Merging #48 (98b218a) into main (424d54b) will increase coverage by 4.40%.
The diff coverage is 100.00%.

📣 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      #48      +/-   ##
==========================================
+ Coverage   77.16%   81.56%   +4.40%     
==========================================
  Files          13       13              
  Lines         289      358      +69     
==========================================
+ Hits          223      292      +69     
  Misses         66       66              
Impacted Files Coverage Δ
src/cosmology/api/__init__.py 100.00% <100.00%> (ø)
src/cosmology/api/_array_api/__init__.py 100.00% <100.00%> (ø)
src/cosmology/api/_components.py 91.26% <100.00%> (+6.51%) ⬆️
src/cosmology/api/_core.py 78.94% <100.00%> (-1.06%) ⬇️
src/cosmology/api/_distances.py 77.77% <100.00%> (+7.18%) ⬆️
src/cosmology/api/_extras.py 92.68% <100.00%> (+4.68%) ⬆️
src/cosmology/api/_standard.py 93.33% <100.00%> (ø)
src/cosmology/api/compat/_core.py 100.00% <100.00%> (ø)
src/cosmology/api/compat/_standard.py 100.00% <100.00%> (ø)

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 ready for review April 13, 2023 21:06
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.

This is a great improvement! Needs a couple of fixes, nothing major.

src/cosmology/api/__init__.py Show resolved Hide resolved
src/cosmology/api/_components.py Outdated Show resolved Hide resolved
src/cosmology/api/_components.py Outdated Show resolved Hide resolved
src/cosmology/api/_components.py Outdated Show resolved Hide resolved
src/cosmology/api/_components.py Outdated Show resolved Hide resolved
tests/api/distances/test_scalefactor.py Outdated Show resolved Hide resolved
tests/api/distances/test_tcmb.py Outdated Show resolved Hide resolved
tests/api/distances/test_tcmb.py Outdated Show resolved Hide resolved
tests/api/extras/test_hubble.py Outdated Show resolved Hide resolved
tests/api/extras/test_hubble.py Outdated Show resolved Hide resolved
@nstarman
Copy link
Contributor Author

I named it TemperatureCMB so that it had the same order as T_CMB.

@nstarman nstarman force-pushed the hierarchy branch 3 times, most recently from fb17673 to d8ac31b Compare April 15, 2023 00:47
nstarman and others added 9 commits April 14, 2023 17:50
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Co-authored-by: Nicolas Tessore <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
@nstarman nstarman requested a review from ntessore April 15, 2023 02:29
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.

One small question whether HoverH0 was intentional or typo, but LGTM!

src/cosmology/api/_extras.py Show resolved Hide resolved
@nstarman nstarman merged commit fbfeb27 into cosmology-api:main Apr 17, 2023
@nstarman nstarman deleted the hierarchy branch April 17, 2023 22: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.

3 participants