Add type annotations to `stats` subpackage #16562

jeffjennings · 2024-06-14T02:57:17Z

Description

Adds type annotations to all modules in stats. Specifically:

adds annotations only to function call signatures (internal and external to classes, including private functions). I included private functions in case of eventual type checking, and because many private functions don't have a docstring.
doesn't attempt annotations for *args or **kwargs** in call signatures
doesn't try to type hint dimensionality of arrays (there are only a handful of cases where they're given in docstrings)
retains type listings in all docstrings

Beyond this, there are ~10 lines with very minor changes to docstrings (adding a missing parameter or return value, updating the type based on the code in the function).

Because not much of the codebase is typed, there may be some errors in type annotations which are hard to determine now. Local tests and doc build pass.

This is my first Astropy PR; I read the developer docs on how to make a contribution, but I appreciate any advice. Thanks!

Related issues (does not fully address / fix them): #15170
Relevant PRs: astropy/astropy-APEs#94 proposes a typing APE

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-06-14T02:57:44Z

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.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions

Welcome to Astropy 👋 and congratulations on your first pull request! 🎉

A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.

If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org.

github-actions · 2024-06-14T02:59:53Z

👋 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?

neutrinoceros · 2024-06-14T06:03:55Z

Awesome. I'm game to review this but I might as well wait until it's undrafted. @jeffjennings CI is all green, is there anything else you're specifically waiting for / need a hand with?

jeffjennings · 2024-06-14T06:37:03Z

Thanks Clément! No if it looks ready for review, go for it.

neutrinoceros · 2024-06-14T06:46:02Z

Awesome. I might give it a look later today if I get stable enough wifi on the train !

pllim · 2024-06-14T08:25:14Z

docs/changes/stats/16562.feature.rst

We have no been adding change log for such changes because they are not visible to users in terms of functionality change (hopefully). Please remove change log and squash it out. Thanks!

Ok I see, it would be nice to update the developer docs to make that clear. E.g. at https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#add-changelog, "Add a changelog fragment briefly describing the change you made..." to "If your change is user-facing, add a changelog fragment briefly describing the change you made...", maybe in #16561

Who performs the squash, me or a reviewer? I assume we wait until a PR is ready to be merged to squash (and include in the squash minor changes addressing reviews)?

@jeffjennings - I have updated the changelog instructions in #16561 to add For changes that impact user-facing functionality, ....

@pllim - I think this change is user-facing. When a user is working with this code, if it has type annotations then their IDE or notebook will magically be more helpful.. So I would say 100% this is user-facing. It also is nice as a developer to get credit for improving astropy with a changelog entry. (As a side-note, I spent at least 16 hours on #16561 and maybe that deserves a changelog entry too so that developers know about it and so I get credit??)

Thank you @taldcroft!

Just to offer my opinion (to you and @pllim), I definitely agree that developers should always get credit for contributions, even those that are internal, and especially when the amount of time and work contributed is large. I think docs contributions (and type annotations) should be a part of this and have changelog entries; the docs are a critical part of the software and are used heavily by both users and developers (e.g. they just taught me the unique steps about contributing to Astropy ahead of my first PR).

Change log is not a form of "credit" so I am a little confused. We have script at release time to properly add contributors to list to be credited and that has nothing to do with change logs.

All the other typing PRs do not have change log, so I don't know why this one for stats needs special exception but I'll defer to @eerovaher and @nstarman and @mhvk who have been involved with a lot of typing related PRs lately.

Change log situation for #16561 should be discussed separately there. I have not looked at that diff carefully yet but I understand the argument (also not related to typing changes).

Thanks! Yes I see your point. Although in cases of retroactively adding type annotations, the person adding the annotations in the call signature lines would be git blamed (which makes sense to me), while the content of the function (and docstring) would in general still be git blamed to the original author (not hiding their contributions). I'd say things like type annotations are real functionality changes, as Tom proposed. Adding them is definitely more manual than auto-formatting.

@pllim - the word "credit" was maybe not perfect, but I meant that the work being done is noticeable in the release notes for people that check change logs. These things matter for both for perception and for informing the community. I'll open an issue now.

For typing in particular, it is a sticky situation because it is tough to do this retroactively and it is not fair if we do not.

As I mention in #16569, I think this warrants not just a changelog entry, but also a what's new. Several users have asked for astropy to be typed, and it is relevant to them that one subpackage now is, and that we are now on a path to include typing more generally.

I don't think any of the previous typing changes were as user-visible, except perhaps cosmology - but that can also get a what's new entry if it is completed.

astropy/stats/bayesian_blocks.py

astropy/stats/sigma_clipping.py

astropy/stats/jackknife.py

nstarman · 2024-06-14T13:40:15Z

@jeffjennings this is really great! I've left some comments, mostly about further type narrowing.

jeffjennings · 2024-06-14T17:16:25Z

Thanks everyone for the review, especially @nstarman! I left review conversations open, but I think they're all addressed (except for squashing). Please resolve any you feel are addressed.

nstarman

Looking good! I noted a few more errant NDArray and Callable

astropy/stats/bayesian_blocks.py

astropy/stats/biweight.py

astropy/stats/funcs.py

nstarman · 2024-06-14T22:26:03Z

astropy/stats/spatial.py

+        self,
+        data: NDArray,
+        radii: NDArray,
+        mode: Literal["none", "translation", "ohser", "var-width", "ripley"] = "none",


Hm. Seems like something Sphinx should have a better fix for. When a type annotation gets very long, as in sigma_clipping.py it's helpful to condense using aliases.

@jeffjennings, if you don't mind, could you try a private alias, like _ModeOps: TypeAlias = Literal["none", "translation", "ohser", "var-width", "ripley"] ?
Then we'll know whether some consolidation can happen, which might help readability in the return annotations in sigma_clipping.py

mhvk · 2024-06-15T07:52:27Z

docs/changes/stats/16562.feature.rst

@@ -0,0 +1 @@
+Adds type annotations to all function (public and private, including class methods) call signatures in ``stats``.


This is a bit TBD, but this should perhaps be 16562.other.rst? Though I can also see the case for .feature... See #16569 (comment) - better wait for discussion before changing it!

I'm happy for this to be other rather than feature. Since no one has objected here or in #16569, I've changed it to other in 9597925

mhvk · 2024-06-15T07:56:38Z

I'd suggest a what's new entry. Maybe something like

Typing in astropy
=================

The ``astropy.stats`` module is now fully typed. This is the first subpackage for
which this the case, and we encourage compatibility tests.

jeffjennings · 2024-06-18T03:24:52Z

I'd suggest a what's new entry. Maybe something like

Typing in astropy
=================

The ``astropy.stats`` module is now fully typed. This is the first subpackage for
which this the case, and we encourage compatibility tests.

Thanks Marten! That's a nice idea. Added in ee02387

jeffjennings · 2024-06-18T03:31:20Z

Thank you again to all the reviewers. I've just addressed all outstanding review points.

neutrinoceros · 2024-06-19T07:26:10Z

I might give it a look later today if I get stable enough wifi on the train !

Four days later: the wifi was not, in fact, stable
Seriously though, I see this got a decent amount of attention already, so maybe my review isn't needed after all. Still happy to go over it if anyone thinks it's worth it, but I'll abstain for now !

jeffjennings · 2024-06-19T13:59:08Z

I might give it a look later today if I get stable enough wifi on the train !

Four days later: the wifi was not, in fact, stable Seriously though, I see this got a decent amount of attention already, so maybe my review isn't needed after all. Still happy to go over it if anyone thinks it's worth it, but I'll abstain for now !

haha no worries!

jeffjennings · 2024-07-02T16:59:39Z

I've rebased on main to resolve a merge conflict (edits to whatsnew), so hopefully this is all good!

adrn · 2024-07-02T17:27:00Z

I think this is good to go! @eerovaher or @nstarman if you are happy with this, since you both have been doing a fair amount of typing work, feel free to merge this.

eerovaher · 2024-07-02T17:53:59Z

I really think that it's the sub-package maintainers who should do the final review and merge. I certainly don't want any type annotations to be added to coordinates without my approval, at least not until we are type checking and have very high coverage.

nstarman · 2024-07-02T18:02:17Z

Agreed, that's what I was waiting on as well.
I'd feel most comfortable with other maintainers merging types into units and cosmology when we have some kind of CI check.

jeffjennings · 2024-07-02T23:21:08Z

That makes sense of course.

@larrybradley, as we talked about on Slack a few weeks ago, I've added type annotations to all of stats. Nathaniel reviewed the changes, but if you'd like to merge this (or add any comments), I'd appreciate it!

larrybradley

I'm not an expert on type annotations, so I'm relying on the thorough reviews of @nstarman , @neutrinoceros , @eerovaher, and @adrn. Thanks again, @jeffjennings, for your work on this and for your first contribution to astropy! 🎉

[ci skip]

pllim · 2024-07-03T13:40:04Z

I took the liberty to do minor edit to what's new entry, so not to give people false hope just on the title by making it more explicit about stats only. If you are okay with it, I will Squash & Merge. Thanks, all!

larrybradley · 2024-07-03T14:03:55Z

@pllim The "what's new" edit looks fine to me.

jeffjennings · 2024-07-03T14:07:20Z

I'm not an expert on type annotations, so I'm relying on the thorough reviews of @nstarman , @neutrinoceros , @eerovaher, and @adrn. Thanks again, @jeffjennings, for your work on this and for your first contribution to astropy! 🎉

Thanks a lot @larrybradley, I appreciate it!

neutrinoceros · 2024-07-03T14:10:43Z

Let me reiterate that I trust other reviewers and am available to provide another review if needed but I did not thoroughly review this (yet) !

neutrinoceros · 2024-07-03T14:11:45Z

Whoops I missed that this was just merged, so clearly you guys don't need my review here 😅
Congratz @jeffjennings !

jeffjennings · 2024-07-03T14:13:34Z

Whoops I missed that this was just merged, so clearly you guys don't need my review here 😅 Congratz @jeffjennings !

Thankz Clément!

adrn · 2024-07-03T15:41:06Z

I really think that it's the sub-package maintainers who should do the final review and merge.

@eerovaher Yes agreed! Sorry, there was out-of-band discussion with @larrybradley -- I didn't mean to suggest we overrule him here :).

nstarman · 2024-07-03T16:38:08Z

Thanks @jeffjennings! A great leap forward for typing in Astropy.

jeffjennings · 2024-07-03T16:44:26Z

Thanks @jeffjennings! A great leap forward for typing in Astropy.

Thank you @nstarman again for reviewing! Especially for a such a big PR. I learned more about typing thanks to your ideas and comments.

* bayesian_blocks: add type hints * biweight: add type hints * circstats: add type hints * funcs: add type hints * histogram: add type hints * info_theory: add type hints * jackknife: add type hints * sigma_clipping: add type hints * spatial: add type hints * setup_package: add type hints * nitpick-exceptions: add ArrayLike type for sphinx * bayesian_blocks.bayesian_blocks: restrict type Co-authored-by: Adrian Price-Whelan <adrianmpw@gmail.com> * bayesian_blocks.bayesian_blocks: better constrain type hint Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com> * bayesian_blocks.FitnessFunc: better type hint Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com> * bayesian_blocks: better constrain type hints * stats: TODO for future type hinting * biweight: add dtype to NDArray type hints * spatial.RipleysKEstimator: add dtype to NDArray type hints * sigma_clipping.SigmaClip: add masked array dtype * jackknife: add generic DType to NDArray type hints * nitpick-exceptions: add np.ma.MaskedArray * stats: type hint syntax for numpy float * docs: add typing entry to whatsnew * stats.funcs: type hint specify float * stats.spatial: add private TypeAlias * stats.biweight._stat_functions: type hint * stats typing: update changelog entry to 'other' * stats.spatial: add typing TODO * stats: NDArray type hint dtype * whatsnew: refine stats typing entry --------- Co-authored-by: Adrian Price-Whelan <adrianmpw@gmail.com> Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com> Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

github-actions bot added Docs stats labels Jun 14, 2024

github-actions bot reviewed Jun 14, 2024

View reviewed changes

jeffjennings marked this pull request as ready for review June 14, 2024 06:37

jeffjennings requested a review from larrybradley as a code owner June 14, 2024 06:37

pllim reviewed Jun 14, 2024

View reviewed changes

pllim added this to the v7.0.0 milestone Jun 14, 2024

pllim added the no-changelog-entry-needed label Jun 14, 2024

pllim requested review from eerovaher and nstarman June 14, 2024 08:25

eerovaher added the typing related to type annotations label Jun 14, 2024

adrn self-requested a review June 14, 2024 11:48

adrn reviewed Jun 14, 2024

View reviewed changes

astropy/stats/bayesian_blocks.py Outdated Show resolved Hide resolved

nstarman reviewed Jun 14, 2024

View reviewed changes

taldcroft mentioned this pull request Jun 14, 2024

Update policy and implementation for changelog entries #16569

Open

nstarman reviewed Jun 14, 2024

View reviewed changes

mhvk reviewed Jun 15, 2024

View reviewed changes

nstarman added the whatsnew-needed label Jun 19, 2024

jeffjennings added 5 commits July 2, 2024 12:35

stats.spatial: add typing TODO

eb2b3e5

nitpick-exceptions: remove np.floating

72b3e7f

stats: NDArray type hint dtype

800a2b0

whatsnew: refine stats typing entry

a909ff6

remove changelog entry

f80d561

jeffjennings force-pushed the typing_stats branch from 84b0098 to f80d561 Compare July 2, 2024 16:36

nstarman mentioned this pull request Jul 2, 2024

docs: add typing coverage page #16660

Open

larrybradley approved these changes Jul 3, 2024

View reviewed changes

Update 7.0.rst

1162447

[ci skip]

pllim merged commit d24e8cb into astropy:main Jul 3, 2024
4 checks passed

nstarman mentioned this pull request Jul 3, 2024

Possible speed-ups for get_grouped_by_powers #16424

Closed

This was referenced Oct 22, 2024

Add SigmaClippedStats class #17221

Merged

TYP: fix incorrect type annotations for optional parameters in stats #17226

Merged

neutrinoceros mentioned this pull request Nov 14, 2024

TYP: simplify type annotation int | float to float #17392

Merged

1 task

neutrinoceros mentioned this pull request Apr 4, 2025

TYP: set up typechecking for units with basedpyright #17918

Draft

1 task

		@@ -0,0 +1 @@
		Adds type annotations to all function (public and private, including class methods) call signatures in ``stats``.

Uh oh!

Add type annotations to stats subpackage #16562

Add type annotations to stats subpackage #16562

Uh oh!

Conversation

jeffjennings commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions bot commented Jun 14, 2024

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 14, 2024

Uh oh!

neutrinoceros commented Jun 14, 2024

Uh oh!

jeffjennings commented Jun 14, 2024

Uh oh!

neutrinoceros commented Jun 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffjennings Jun 14, 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!

jeffjennings Jun 14, 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!

jeffjennings Jun 14, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nstarman commented Jun 14, 2024

Uh oh!

jeffjennings commented Jun 14, 2024

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffjennings Jun 18, 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!

mhvk commented Jun 15, 2024

Uh oh!

jeffjennings commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Add type annotations to `stats` subpackage #16562

Add type annotations to `stats` subpackage #16562

jeffjennings commented Jun 14, 2024 •

edited

Loading

jeffjennings Jun 14, 2024 •

edited

Loading

jeffjennings Jun 14, 2024 •

edited

Loading

jeffjennings Jun 14, 2024 •

edited

Loading

jeffjennings Jun 18, 2024 •

edited

Loading

jeffjennings commented Jun 18, 2024 •

edited

Loading

neutrinoceros commented Jun 19, 2024 •

edited

Loading

eerovaher commented Jul 2, 2024 •

edited

Loading