Repository navigation
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.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
b5a7998 to
df807a5
Compare
99bf161 to
1500054
Compare
1500054 to
8194bc5
Compare
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
099554e to
44efc73
Compare
There was a problem hiding this comment.
Pull request overview
This PR makes FLRW.comoving_transverse_distance support the public 2-argument calling form (z1, z2), aligning it with existing 2-argument distance APIs in astropy.cosmology.
Changes:
- Update
FLRW.comoving_transverse_distanceto accept an optional second redshift argument (z2) and implement the(z1, z2)behavior directly. - Update cosmology test suite to use the public 2-argument method instead of the former private helper.
- Add a cosmology changelog fragment documenting the new capability.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
docs/changes/cosmology/18803.feature.rst |
Documents the new optional z2 argument for comoving_transverse_distance. |
astropy/cosmology/_src/tests/funcs/test_funcs.py |
Switches round-trip tests to use the public 2-argument API. |
astropy/cosmology/_src/tests/flrw/test_lambdacdm.py |
Updates transverse distance tests to call the public 2-argument API. |
astropy/cosmology/_src/flrw/base.py |
Implements the optional-z2 signature and updates related call sites/docstrings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
pre-commit.ci autofix |
|
@neutrinoceros, ready :) |
f023eb1 to
2b2170b
Compare
|
@neutrinoceros this PR simplifies the |
| def comoving_transverse_distance(self, z: u.Quantity | ArrayLike, /) -> u.Quantity: | ||
| r"""Comoving transverse distance in Mpc at a given redshift. | ||
| def comoving_transverse_distance( | ||
| self, z: _InputT, z2: _InputT | None = None, / |
There was a problem hiding this comment.
The annotations look either incorrect or at odds with how the arguments are actually handled. A type var implies that, as long as z2 != None, z2 and z1,always adhere to some common type (or interface); but then z1 is conditionally set to 0.0 (without any regard to z2's type).
There was a problem hiding this comment.
I think relaxing the type annotations to be _InputT | float should work. WDYT?
There was a problem hiding this comment.
unioning a typevar seems like an antipattern to me: in most scenarios, it just kills the benefits from a typevar, and in the rare cases it might be correct, it still looks fishy enough that I'd rather avoid it to save mental bandwidth in readers.
What type checker (if any) are you using to check these ?
There was a problem hiding this comment.
In this case we always want to be able to accept floats. We also want to support higher types. What I think we want to discourage is a lot of type mixing.
I think that's a TypeVar|float.
I use mypy on Astropy and ty on pretty much everything else.
WDYT?
There was a problem hiding this comment.
so TypeVar|float basically means "this can always be a float, but if it isn't then it needs to be consistent with other non-float variables that have this type" ? I think I get the intention but I'm not sure that type checkers understand this (or that it's supported by the spec). Do you have any source to back it up ? I'll play around with it and see what happens.
I use mypy on Astropy and ty on pretty much everything else.
Out of curiosity, I ran ty check astropy/cosmology and pyrefly check astropy/cosmology. ty seems to pick up many problems pyright does not, though I don't know wether they are real of false positives.
There was a problem hiding this comment.
I've fixed all errors to do with cosmoving_transverse_distance
ty check astropy/cosmology 2>&1 | grep -A 5 -B 5 "comoving_transverse_distance"
neutrinoceros
left a comment
There was a problem hiding this comment.
sorry for taking so long to reply. On paper this is perfectly fine to me. The only important thing to fix IMO are the type annotations.
2b2170b to
ad9afdd
Compare
|
Ping @neutrinoceros for thoughts. |
|
@neutrinoceros all ty complaints about |
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
e1b8e3c to
6689213
Compare
neutrinoceros
left a comment
There was a problem hiding this comment.
Only minor comments here. LGTM otherwise
|
|
||
| @pytest.mark.skipif(not HAS_SCIPY, reason="test requires scipy") | ||
| def test_comoving_transverse_distance_z1z2(): | ||
| def test_comoving_transverse_distance(): |
There was a problem hiding this comment.
I think I would actually keep the original test name here
There was a problem hiding this comment.
I like the new name. Unified function w/ 2 args.
Co-authored-by: Clément Robert <[email protected]>
|
Thanks @neutrinoceros for the reviews! |
|
@neutrinoceros / @pllim can you confirm the CI failures are unrelated. Then I can bypass and merge. :) Thanks. |
|
I confirm. These are known failures unrelated to your PR. |
Make the 2-argument form of
comoving_transverse_distancepublic.Requires #18800, #18807