Cosmo: type annotate the optimization functions by nstarman · Pull Request #15853 · astropy/astropy

nstarman · 2024-01-10T20:52:58Z

Followup to #15852

Plz squash.

github-actions · 2024-01-10T20:53:23Z

github-actions · 2024-01-10T20:53:27Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

nstarman · 2024-01-10T22:06:14Z

Need Unpack from typing_extensions.

github-actions · 2024-06-18T05:41:04Z

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.

nstarman · 2024-12-04T22:41:10Z

I'm hesitant to add the types ZAtValueKWArgs and CustomMethod to Cosmology's public API.
Private type hints are still useful to IDEs, just not the docs.
@eerovaher WDYK? Is there a balance that can be struck?

eerovaher · 2024-12-05T22:59:12Z

In a greenfield project I would insist on documenting all types that are used in the public API, but astropy is not a greenfield project. Here it is better to follow what I've seen referred to as the Scout rule, i.e. our pull requests should leave the code in a better state than it was before. Introducing the new types doesn't make the existing docstrings any worse, but it does make the code friendlier towards IDEs and typing-aware developers.

nstarman · 2024-12-09T01:50:36Z

Test failures unrelated.

eerovaher · 2024-12-09T17:11:21Z



+# TODO: it would be nice to upstream this to scipy and then use it here.
+class _CustomMethod(Protocol):


Why is this implemented as a Protocol with a __call__() method instead of something like

_CustomMethod: TypeAlias = Callable[..., OptimizeResult]

And is _CustomMethod the best name for this?

Because the structure of _CustomMethod is more specific than that. It has to have a signature like
(fun: Callable[..., Any], args: tuple[Any, ...], **kwargs: Any) -> OptimizeResult.
But I agree about a better name!

Signed-off-by: nstarman <[email protected]>

eerovaher · 2024-12-11T15:06:53Z

-    from typing import Any, Literal
+    from typing import Literal
+
+    from typing_extensions import Unpack


Unpack is used in a few places to unpack **atzkw. That functionality was added to Unpack in Python 3.12 (see PEP 692 – Using TypedDict for more precise **kwargs typing). I'm willing to guess that the reason for importing Unpack from typing_extensions instead of typing is to make that functionality available in Python 3.11. But apparently typing_extensions.Unpack only backports the changes to Unpack.__repr__(), not the ability to unpack **kwargs, which means that the new type annotations are valid for Python 3.12 and later, but not for Python 3.11.

Unpack has been in typing since 3.11.

In Python 3.11 typing.Unpack can only be used to unpack tuples.

Then does it come through as an Any? My mypy isn't flagging any issues.

I haven't tested this with Python 3.11.

So what's the step forward? Merge as-is and wait for mypy checking of cosmology?

It is possible that I am misunderstanding the typing_extensions documentation and this is actually not a problem. Someone should type check this with Python 3.11 to find out for sure.

I looked into it more. It doesn't do it. Sigh. I kind of don't want to change it, but should. Just defeats the point of adding _ZAtValueKWArgs

Working around it the gross way.

That's not the prettiest code, but it has no effect at runtime, we only need it until we drop support for Python 3.11 and Ruff will not let us forget about it when we do.

Signed-off-by: nstarman <[email protected]>

nstarman added the no-changelog-entry-needed label Jan 10, 2024

nstarman added this to the v6.1.0 milestone Jan 10, 2024

github-actions Bot added the cosmology label Jan 10, 2024

eerovaher reviewed Jan 11, 2024

View reviewed changes

Comment thread astropy/cosmology/funcs/optimize.py Outdated

This was referenced Jan 11, 2024

change convention for typing_extensions #15867

Closed

change convention for typing_extensions #15870

Merged

nstarman force-pushed the cosmo-typing-optimize branch from 8e1a839 to c7b3fd6 Compare January 11, 2024 20:30

nstarman force-pushed the cosmo-typing-optimize branch from c7b3fd6 to de274d3 Compare January 20, 2024 04:23

astrofrog modified the milestones: v6.1.0, v7.0.0 Apr 4, 2024

mhvk added the typing related to type annotations label May 30, 2024

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Jun 18, 2024

nstarman added keep-open and removed Close? Tell stale bot that this issue/PR is stale labels Jun 19, 2024

nstarman force-pushed the cosmo-typing-optimize branch from de274d3 to 7969a18 Compare October 9, 2024 20:08

saimn modified the milestones: v7.0.0, v7.1.0 Oct 26, 2024

nstarman force-pushed the cosmo-typing-optimize branch 2 times, most recently from e6e537b to 544c68c Compare December 4, 2024 21:57

nstarman requested a review from eerovaher December 5, 2024 21:31

eerovaher reviewed Dec 6, 2024

View reviewed changes

Comment thread astropy/cosmology/funcs/optimize.py

Comment thread astropy/cosmology/funcs/optimize.py Outdated

Comment thread astropy/cosmology/units.py Outdated

nstarman force-pushed the cosmo-typing-optimize branch from 544c68c to b9cfdc7 Compare December 9, 2024 01:35

nstarman marked this pull request as ready for review December 9, 2024 01:50

nstarman requested a review from a team as a code owner December 9, 2024 01:50

nstarman requested a review from eerovaher December 9, 2024 01:50

eerovaher reviewed Dec 9, 2024

View reviewed changes

nstarman added 2 commits December 9, 2024 18:03

type annotate cosmology…optimize.py

a63d879

Signed-off-by: nstarman <[email protected]>

update type annotations in units from optimize

011c6c9

Signed-off-by: nstarman <[email protected]>

nstarman force-pushed the cosmo-typing-optimize branch from b9cfdc7 to 011c6c9 Compare December 9, 2024 23:03

nstarman requested a review from eerovaher December 9, 2024 23:03

eerovaher reviewed Dec 11, 2024

View reviewed changes

nstarman requested a review from eerovaher December 12, 2024 16:13

fix: py3.11 compat for Unpack

12a8177

Signed-off-by: nstarman <[email protected]>

eerovaher approved these changes Dec 13, 2024

View reviewed changes

eerovaher merged commit ca602e3 into astropy:main Dec 13, 2024



		# TODO: it would be nice to upstream this to scipy and then use it here.
		class _CustomMethod(Protocol):

Uh oh!

Conversation

nstarman commented Jan 10, 2024

Uh oh!

github-actions Bot commented Jan 10, 2024

Uh oh!

github-actions Bot commented Jan 10, 2024

Uh oh!

nstarman commented Jan 10, 2024

Uh oh!

Uh oh!

github-actions Bot commented Jun 18, 2024

Uh oh!

nstarman commented Dec 4, 2024

Uh oh!

eerovaher commented Dec 5, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nstarman commented Dec 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nstarman Dec 9, 2024 •

edited

Loading