Skip to content

Revert Gaussian test change from #14718 - #14771

Merged
pllim merged 3 commits into
astropy:mainfrom
WilliamJamieson:test/fix_bbox
May 6, 2023
Merged

Revert Gaussian test change from #14718#14771
pllim merged 3 commits into
astropy:mainfrom
WilliamJamieson:test/fix_bbox

Conversation

@WilliamJamieson

@WilliamJamieson WilliamJamieson commented May 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

I can no longer reproduce the error skipped by #14718 for the Gaussian locally, this PR demonstrates that skipping this was unnecessary. However, there are new issues... This PR goes on to correct the remaining modeling issues as those were only related to slight numerical changes that required slightly changing tolerances.

I believe this change maybe due to numpy/numpy#23721 or numpy/numpy#23695 updating some underlying things.

Fixes #14766

@github-actions

github-actions Bot commented May 6, 2023

Copy link
Copy Markdown
Contributor

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 "When to rebase and squash commits".
  • 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?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • 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

github-actions Bot commented May 6, 2023

Copy link
Copy Markdown
Contributor

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

@WilliamJamieson WilliamJamieson changed the title TST: if numpy 1.25.dev error has been fixed for Gaussian Revert Gaussian test change from #14718 May 6, 2023
@WilliamJamieson
WilliamJamieson requested review from mhvk and pllim May 6, 2023 12:43
@WilliamJamieson
WilliamJamieson marked this pull request as ready for review May 6, 2023 12:43
@WilliamJamieson
WilliamJamieson requested a review from a team as a code owner May 6, 2023 12:43
@WilliamJamieson WilliamJamieson added this to the v5.2.3 milestone May 6, 2023
@WilliamJamieson WilliamJamieson mentioned this pull request May 6, 2023
Closed
mhvk
mhvk approved these changes May 6, 2023
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bit odd that Gaussian now is OK, but the tests pass, so hopefully it remains OK. The other changes also look good.

pllim
pllim approved these changes May 6, 2023
View reviewed changes

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These numpy changes are painful for me. Hard to keep track what is what. At least the remaining failures are not modeling, so LGTM. Thanks!

@pllim pllim added the numpy-dev label May 6, 2023
@pllim
pllim enabled auto-merge May 6, 2023 14:53
@pllim
pllim merged commit 40ec806 into astropy:main May 6, 2023
@meeseeksmachine meeseeksmachine mentioned this pull request May 6, 2023
Closed
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request May 6, 2023
@pllim @meeseeksmachine
Backport PR astropy#14771: Revert Gaussian test change from astropy#1…
bd8b0da 
…4718
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request May 6, 2023
@pllim @meeseeksmachine
Backport PR astropy#14771: Revert Gaussian test change from astropy#1…
fbea9e4 
…4718
@meeseeksmachine meeseeksmachine mentioned this pull request May 6, 2023
Closed
@WilliamJamieson WilliamJamieson mentioned this pull request May 6, 2023
Merged
@WilliamJamieson

WilliamJamieson commented May 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Bit odd that Gaussian now is OK, but the tests pass, so hopefully it remains OK. The other changes also look good.

@mhvk see #14718 (comment) for my analysis of what specifically is going on. Its either a small numerical change (maybe numpy/numpy#23721?), a thresholding fix (maybe numpy/numpy#23695?), or something else.

I don't precisely know when the new numpy dev wheels were made and what changes were in each version of the wheels to track it down further... I don't think its critical for me to find the changes which caused then fixed the Gaussian issue from numpy either.

@pllim

pllim commented May 6, 2023

Copy link
Copy Markdown
Member

Going to kick off backports again so it picks up all the diff.

@meeseeksdev backport to v5.2.x

@meeseeksdev backport to v5.3.x

@lumberbot-app

lumberbot-app Bot commented May 6, 2023

Copy link
Copy Markdown

Could not push to auto-backport-of-pr-14771-on-v5.2.x due to error, aborting.

@lumberbot-app

lumberbot-app Bot commented May 6, 2023

Copy link
Copy Markdown

Could not push to auto-backport-of-pr-14771-on-v5.3.x due to error, aborting.

@pllim

pllim commented May 6, 2023

Copy link
Copy Markdown
Member

C'mone.

@pllim

pllim commented May 6, 2023

Copy link
Copy Markdown
Member

@meeseeksdev backport to v5.3.x

@lumberbot-app

lumberbot-app Bot commented May 6, 2023

Copy link
Copy Markdown

Could not push to auto-backport-of-pr-14771-on-v5.3.x due to error, aborting.

pllim added a commit to pllim/astropy that referenced this pull request May 6, 2023
@pllim
Merge pull request astropy#14771 from WilliamJamieson/test/fix_bbox
4af210f 
Revert Gaussian test change from astropy#14718
@pllim pllim mentioned this pull request May 6, 2023
Merged
pllim added a commit to pllim/astropy that referenced this pull request May 6, 2023
@pllim
Merge pull request astropy#14771 from WilliamJamieson/test/fix_bbox
edf6848 
Revert Gaussian test change from astropy#14718
@mhvk

mhvk commented May 6, 2023

Copy link
Copy Markdown
Contributor

Thanks for the explanation, @WilliamJamieson!

@pllim pllim mentioned this pull request May 6, 2023
Merged
pllim added a commit to pllim/astropy that referenced this pull request Jun 14, 2023
@pllim
TST: Revert tol change in astropy#14771
b342fa8 
from numpy cos/sin changes that have since been reverted in 1.25.dev
@pllim pllim mentioned this pull request Jun 14, 2023
Merged
pllim added a commit to pllim/astropy that referenced this pull request Jun 14, 2023
@pllim
TST: Revert tol change in astropy#14771
f9299fb 
from numpy cos/sin changes that have since been reverted in 1.25.dev
pllim added a commit to pllim/astropy that referenced this pull request Jun 19, 2023
@pllim
TST: Revert tol change in astropy#14771
b30da2e 
from numpy cos/sin changes that have since been reverted in 1.25.dev
pllim added a commit to pllim/astropy that referenced this pull request Jun 20, 2023
@pllim
TST: Revert tol change in astropy#14771
0deb009 
from numpy cos/sin changes that have since been reverted in 1.25.dev
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim approved these changes
@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

modeling no-changelog-entry-needed numpy-dev

Projects

None yet

Milestone

v5.2.3

Development

Successfully merging this pull request may close these issues.

Investigate Gaussian2D bounding box shape mismatch in numpy 1.25 or later

3 participants

@WilliamJamieson @pllim @mhvk