Skip to content

Fix for SimplexLSQFitter with scipy 1.9.0rc1#13413

Merged
pllim merged 1 commit intoastropy:mainfrom
WilliamJamieson:bugfix/scipy1.9.0rc1
Jun 28, 2022
Merged

Fix for SimplexLSQFitter with scipy 1.9.0rc1#13413
pllim merged 1 commit intoastropy:mainfrom
WilliamJamieson:bugfix/scipy1.9.0rc1

Conversation

@WilliamJamieson
Copy link
Contributor

@WilliamJamieson WilliamJamieson commented Jun 28, 2022 •
edited by pllim
Loading

Description

The scipy 1.9.0rc1 release is causing a warning to a fitting test, see #13400. This PR attempts to resolve this issue via increasing the iteration limit.

Fixes #13400

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • 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.
  • 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 astropy-bot check might be missing; 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.

@WilliamJamieson WilliamJamieson requested a review from pllim June 28, 2022 16:17
@WilliamJamieson WilliamJamieson requested a review from a team as a code owner June 28, 2022 16:17
WilliamJamieson
WilliamJamieson commented Jun 28, 2022
View reviewed changes
astropy/modeling/tests/test_fitters.py

if isinstance(fitter, SimplexLSQFitter):
kwargs = {'maxiter': 77, 'verblevel': 1, 'acc': 1e-6}
kwargs = {'maxiter': 79, 'verblevel': 1, 'acc': 1e-6}
Copy link
Contributor Author

@WilliamJamieson WilliamJamieson Jun 28, 2022

Choose a reason for hiding this comment

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

Note that any maxiter >= 79 will "fix" the failure with scipy 1.9.0rc1; however, I am unsure if this is addressing the fundamental issue.

@WilliamJamieson
Copy link
Contributor Author

WilliamJamieson commented Jun 28, 2022

Looking into this further, I have determined that scipy/scipy#15259, changed a simple print to raising an actual warning. We were already handling this and raising our own warning here:

astropy/astropy/modeling/optimizers.py

Lines 242 to 245 in 1e75f29

if self.fit_info['exit_mode'] == 2:
warnings.warn("The fit may be unsuccessful; "
"Maximum number of iterations reached.",
AstropyUserWarning)
Which is being filtered by the existing test here:

astropy/astropy/modeling/tests/test_fitters.py

Line 1004 in 1e75f29

@pytest.mark.filterwarnings('ignore:The fit may be unsuccessful')

@pllim pllim added this to the v5.0.5 milestone Jun 28, 2022
pllim
pllim approved these changes Jun 28, 2022
View reviewed changes
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Better get this in before the RC comes out.

If you are worried about not fixing the fundamental issue, please open a follow-up issue to track it.

Thanks!

@pllim pllim added the Extra CI Run cron CI as part of PR label Jun 28, 2022
@pllim
Copy link
Member

pllim commented Jun 28, 2022

William reminded me that I should probably check against the RC job, so I enabled cron job but cancelling the irrelevant ones.

@pllim
Copy link
Member

pllim commented Jun 28, 2022

RC job passed. Thanks!

@pllim pllim merged commit 2f4b3d2 into astropy:main Jun 28, 2022
@lumberbot-app
Copy link

lumberbot-app bot commented Jun 28, 2022

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout v5.0.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 2f4b3d2e51e22d2b4309b9cd74aa723a49cfff99
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #13413: Fix for SimplexLSQFitter with scipy 1.9.0rc1'
  1. Push to a named branch:
git push YOURFORK v5.0.x:auto-backport-of-pr-13413-on-v5.0.x
  1. Create a PR against branch v5.0.x, I would have named this PR:

"Backport PR #13413 on branch v5.0.x (Fix for SimplexLSQFitter with scipy 1.9.0rc1)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Jun 28, 2022
@pllim @meeseeksmachine
Backport PR astropy#13413: Fix for SimplexLSQFitter with scipy 1.9.0rc1
a8083f2
@meeseeksmachine meeseeksmachine mentioned this pull request Jun 28, 2022
Merged
@pllim
Copy link
Member

pllim commented Jun 28, 2022

@WilliamJamieson , any chance you could manually backport this to v5.0.x ? Thanks!

WilliamJamieson added a commit to WilliamJamieson/astropy that referenced this pull request Jun 29, 2022
@WilliamJamieson
Merge pull request astropy#13413 from WilliamJamieson/bugfix/scipy1.9…
1b2f754 
….0rc1

Fix for SimplexLSQFitter with scipy 1.9.0rc1
@WilliamJamieson WilliamJamieson mentioned this pull request Jun 29, 2022
Merged
10 tasks
pllim added a commit that referenced this pull request Jun 29, 2022
@pllim
Merge pull request #13415 from WilliamJamieson/backport/13413
241b012 
Manual backport of #13413
pllim added a commit that referenced this pull request Jun 29, 2022
@pllim
Merge pull request #13414 from meeseeksmachine/auto-backport-of-pr-13…
3b84c18 
…413-on-v5.1.x

Backport PR #13413 on branch v5.1.x (Fix for SimplexLSQFitter with scipy 1.9.0rc1)
@WilliamJamieson WilliamJamieson deleted the bugfix/scipy1.9.0rc1 branch June 29, 2022 14:23
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

Assignees

No one assigned

Labels

Extra CI Run cron CI as part of PR modeling no-changelog-entry-needed

Projects

None yet

Milestone

v5.0.5

Development

Successfully merging this pull request may close these issues.

TST: test_fitters_interface[SimplexLSQFitter] failed with scipy 1.9.0rc1

2 participants

@WilliamJamieson @pllim