8000 TYP: fix incorrect type annotations for optional parameters in stats by neutrinoceros · Pull Request #17226 · astropy/astropy · GitHub
[go: up one dir, main page]
Skip to content

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

Merged
larrybradley merged 1 commit intoastropy:mainfrom
neutrinoceros:stats/typ/fix_incorrect_annotations
Oct 22, 2024
Merged

TYP: fix incorrect type annotations for optional parameters in stats#17226
larrybradley merged 1 commit intoastropy:mainfrom
neutrinoceros:stats/typ/fix_incorrect_annotations

Conversation

@neutrinoceros
Copy link
Contributor
@neutrinoceros neutrinoceros commented Oct 22, 2024

Description

This is a direct follow up to #16562 were some confusion around optional parameters and the meaning of | None as a type hint crept in. I initially explained the issue in a comment that I'll reproduce here for convenience:

what does it mean to pass sigma=None ? This looks like it could be a misunderstanding of the meaning of this type-annotation: | None is not synonymous with "has a default value" (though the name of typing.Optional may be the source of confusion), but really means "this parameter can be None".

I grepped for the regular expression (float|int) \| None = [\d\.] to discover incorrect cases (also a couple correct ones !). Inspecting the code for where these parameters were used quickly revealed that in 22 cases, passing None wouldn't make sense and would immediately trigger an exception. In some cases, the docstrings also provided clear indications that the annotations were not correct: some parameters were documented as int, or None (optional) next to others documented as int (optional).

Discovered while reviewing #17221

cc @jeffjennings

  • 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.

@neutrinoceros neutrinoceros added this to the v7.0.0 milestone Oct 22, 2024
@neutrinoceros neutrinoceros added no-changelog-entry-needed typing related to type annotations labels Oct 22, 2024
@github-actions github-actions bot added the stats label Oct 22, 2024
@github-actions
Copy link
Contributor
github-actions bot commented Oct 22, 2024

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
Copy link
Contributor
github-actions bot commented Oct 22, 2024

👋 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 neutrinoceros marked this pull request as ready for review October 22, 2024 09:11
@neutrinoceros neutrinoceros mentioned this pull request Oct 22, 2024
Merged
1 task
larrybradley
larrybradley approved these changes Oct 22, 2024
View reviewed changes
Copy link
Member
@larrybradley larrybradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @neutrinoceros!

@larrybradley
Copy link
Member
larrybradley commented Oct 22, 2024

@pllim I can't merge this because the change log check is stuck. How to fix?

larrybradley
larrybradley reviewed Oct 22, 2024
View reviewed changes
larrybradley
larrybradley requested changes Oct 22, 2024
View reviewed changes
Copy link
Member
@larrybradley larrybradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few others that need to be updated.

@neutrinoceros neutrinoceros force-pushed the stats/typ/fix_incorrect_annotations branch from 2b17c3e to 3526722 Compare October 22, 2024 15:38
@larrybradley larrybradley merged commit 57eec9f into astropy:main Oct 22, 2024
@neutrinoceros neutrinoceros deleted the stats/typ/fix_incorrect_annotations branch October 22, 2024 15:57
@eerovaher
Copy link
Member
eerovaher commented Oct 22, 2024

The project is not maintaining configuration for any type checkers and the small fraction of annotated code in astropy limits the usefulness of type checkers anyways. This means that the correctness of the annotations is mostly enforced by human reviewers. It is to be expected that in a pull request as large as #16562 some mistakes slip through. This pull request found some of them, but I wouldn't be surprised if we find more in the future. The presence of such mistakes is the main reason why I don't want to announce anything related to annotations publicly until we are type checking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments

@larrybradley larrybradley larrybradley approved these changes

@jeffjennings jeffjennings Awaiting requested review from jeffjennings

Assignees

No one assigned

Labels

no-changelog-entry-needed stats typing related to type annotations

Projects

None yet

Milestone

v7.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@neutrinoceros @larrybradley @eerovaher @pllim
0