Repository navigation
feat(cosmo): curvature component - #18232
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
fbd5cbb to
833283b
Compare
d584836 to
a4f9189
Compare
|
Hm. @eerovaher. I'm seeing some more fun with sphinx and typing. Let's figure it out. |
|
You shouldn't have problems with Sphinx anymore now that we don't include type annotations in the API docs. |
a4f9189 to
60e01a1
Compare
|
Very nice on one hand. But it's a shame we don't see what's broken (in sphinx) and why. Flying a bit blind. |
db45719 to
38bd593
Compare
38bd593 to
adef243
Compare
d954916 to
c246197
Compare
c246197 to
7757eeb
Compare
|
Thanks @eerovaher for the review. I think this is ready for round 2. |
|
Ping @mhvk @eerovaher for hopefully a final review. |
|
Now blocked by #18546. |
Why? |
|
6d3b722 to
eb8f0cc
Compare
eb8f0cc to
8f89f5c
Compare
010ef06 to
2a14f0f
Compare
|
Ping @mhvk @eerovaher for hopefully a final review. |
github-project-automation
Bot
moved this from In Progress
to Reviewer approved
in Cosmology, the Expansion
| "astropy/cosmology/_src/io/builtin/mapping.py = python>=3.12", # not PYTHON_LT_3_12 (PR 17277) | ||
| "astropy/cosmology/_src/io/builtin/row.py = python>=3.12", # not PYTHON_LT_3_12 (PR 17277) | ||
| "astropy/cosmology/_src/io/builtin/table.py = python>=3.12", # not PYTHON_LT_3_12 (PR 17277) | ||
| "astropy/cosmology/_src/traits/curvature.py = numpy>=2", # (PR 18232) |
There was a problem hiding this comment.
| "astropy/cosmology/_src/traits/curvature.py = numpy>=2", # (PR 18232) | |
| "astropy/cosmology/_src/traits/curvature.py = numpy>=2", # not NUMPY_LT_2_0 |
Mentioning NUMPY_LT_2_0 is useful for grepping. And although I can see that many of the surrounding lines do mention pull requests in their comments, I don't see why that would be useful. If I want to find out when this line was added then I can use git blame.
There was a problem hiding this comment.
SGTM. I'll do both. I find direct references often more convenient than git-blame. But both work.
There was a problem hiding this comment.
The usefulness of git blame is limited by the quality of commit history and commit messages. Although it is true that in astropy the quality of both is rather low, this sad state of affairs should not be acceptable.
There was a problem hiding this comment.
As long as we keep using github, the quality of the commit messages will remain abysmal - it does not encourage writing anything useful. And within the astropy project, there has effectively been a collective decision to not worry about it overly, as improving it costs time that is better spent elsewhere. In that respect, in my experience, one usually just wants to improve the current state, and thus only rarely is there a need for the detailed history. That suggests it is not worth spending a large amount of effort on improving it (admittedly, my only point of comparison is numpy, which is better than astropy, but not by that much).
In this particular case, the most helpful part is not NUMPY_LT_2_0, as it will help our future selves. The rest is a breadcrumb that may or may not be useful.
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
2a14f0f to
bd26f17
Compare
eerovaher
left a comment
eerovaher
left a comment
There was a problem hiding this comment.
Extracting closely related code to separate trait-like classes enables sharing code that should be shared without also having to share code that does not have to be shared. Furthermore, it makes it possible to write and document functions that require its inputs to implement some specific trait without limiting the inputs needlessly. astropy already has a few examples of trait-like classes, such as astropy.utils.shapes.NDArrayShapeMethods or astropy.utils.shapes.ShapedLikeNDArray. It is good to see that cosmology is now making use of that pattern too.
github-project-automation
Bot
moved this from Reviewer approved
to Done
in Cosmology, the Expansion
Working towards #17150
@preshanth hopefully this PR helps!