8000 TYP: simplify type annotation `int | float` to `float` by neutrinoceros · Pull Request #17392 · astropy/astropy · GitHub
[go: up one dir, main page]
Skip to content

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

Merged
larrybradley merged 1 commit intoastropy:mainfrom
neutrinoceros:stats/typ/fix_incorrect_annotations_floats
Nov 14, 2024
Merged

TYP: simplify type annotation int | float to float#17392
larrybradley merged 1 commit intoastropy:mainfrom
neutrinoceros:stats/typ/fix_incorrect_annotations_floats

Conversation

@neutrinoceros
Copy link
Contributor
@neutrinoceros neutrinoceros commented Nov 14, 2024

Description

Noticed this while working on #17391
follow up to #16562
Unfortunately I can't remember the source for this piece of advice (maybe that's actually specific to mypy), but type checkers treat int as a subtype of float, so the union int | float can be simplified to float in annotations.

  • 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
Copy link
Contributor
github-actions bot commented Nov 14, 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 Nov 14, 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 modified the milestones: v7.0.0, v7.1.0 Nov 14, 2024
@neutrinoceros neutrinoceros changed the title TYP: simplify type annotation (int | float) to float TYP: simplify type annotation int | float to float Nov 14, 2024
@neutrinoceros neutrinoceros marked this pull request as ready for review November 14, 2024 11:45
larrybradley
larrybradley approved these changes Nov 14, 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.

LGTM, thanks

@larrybradley larrybradley merged commit 4a8db39 into astropy:main Nov 14, 2024
@neutrinoceros neutrinoceros deleted the stats/typ/fix_incorrect_annotations_floats branch November 14, 2024 14:52
@eerovaher eerovaher mentioned this pull request Nov 14, 2024
Merged
1 task
eerovaher
eerovaher reviewed Nov 14, 2024
View reviewed changes
Copy link
Member
@eerovaher eerovaher left a comment

Choose a reason for hiding this comment

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

This pull request should not have been merged in this state, but because of how little time this was open I didn't have a chance to comment before merging. I have already opened a pull request to undo the changes to units.

astropy/stats/histogram.py
a: ArrayLike,
bins: int
| list[int | float]
| list[float]
Copy link
Member
@eerovaher eerovaher Nov 14, 2024

Choose a reason for hiding this comment

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

list is a mutable collection, so it is invariant, not covariant (see PEP 483 (The Theory of Type Hints)). This means that although int is a subtype of float, list[int] is not a subtype of list[float]. Using a covariant type such as Sequence[float] would be better, but it might be even better to look up annotations from np.histogram_bin_edges(). The same goes for histogram() too.

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

Reviewers

@eerovaher eerovaher eerovaher left review comments

@larrybradley larrybradley larrybradley approved these changes

Assignees

No one assigned

Labels

no-changelog-entry-needed stats units

Projects

None yet

Milestone

v7.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@neutrinoceros @larrybradley @eerovaher
0