8000 ENH: `optimize.root`: add warning for invalid inner parameters in `newton_krylov` method by swstkm · Pull Request #22809 · scipy/scipy · GitHub
[go: up one dir, main page]
Skip to content

ENH: optimize.root: add warning for invalid inner parameters in newton_krylov method #22809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
May 14, 2025
Merged

ENH: optimize.root: add warning for invalid inner parameters in newton_krylov method #22809

merged 14 commits into from
May 14, 2025

Conversation

swstkm
Copy link
Contributor
@swstkm swstkm commented Apr 7, 2025

Reference issue

Closes #21986

What does this implement/fix?

Improves parameter validation in Krylov solver in optimize.root by raising warnings for invalid inner parameters but ensuring no warnings for user-provided callable methods.

Additional information

@swstkm swstkm requested a review from andyfaff as a code owner April 7, 2025 10:32
@github-actions github-actions bot added scipy.optimize enhancement A new feature or improvement labels Apr 7, 2025
@lucascolley lucascolley self-requested a review April 7, 2025 11:04
lucascolley
lucascolley reviewed Apr 7, 2025
View reviewed changes
Copy link
Member
@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

Thanks @swstkm, this looks great!

@janosg how does this look from a downstream perspective?

< 8000 details-toggle>
@lucascolley lucascolley added this to the 1.16.0 milestone Apr 7, 2025
@swstkm @lucascolley
Replace assert_ statements with assert
f6a68bd 
Co-authored-by: Lucas Colley <lucas.colley8@gmail.com>
@janosg
Copy link
janosg commented Apr 7, 2025

Hi @lucascolley, I like it. This is definitely a nice improvement 🙂

The warning message currently says:

"Please check inner method documentation for valid options."

I think we could go beyond that, either by making a "did you mean xyz" proposal using difflib or by just adding the valid options into the warning message. The information needed for this is already available, so the implementation would be really quick.

@lucascolley
Copy link
Member

thanks @janosg !

I think we could go beyond that, either by making a "did you mean xyz" proposal using difflib or by just adding the valid options into the warning message. The information needed for this is already available, so the implementation would be really quick.

would you be willing to have a go at this @swstkm ?

@swstkm
Copy link
Contributor Author
swstkm commented Apr 7, 2025

@lucascolley @janosg does this do the job?

j-bowhay
j-bowhay reviewed Apr 7, 2025
View reviewed changes
lucascolley
lucascolley reviewed Apr 7, 2025
View reviewed changes
@swstkm @lucascolley
Update scipy/optimize/_nonlin.py to resolve linter issues
5c7a867 
Co-authored-by: Lucas Colley <lucas.colley8@gmail.com>
j-bowhay
j-bowhay reviewed Apr 9, 2025
View reviewed changes
j-bowhay
j-bowhay reviewed Apr 9, 2025
View reviewed changes
@swstkm @j-bowhay
Apply suggestions from code review
a1aa0f9 
Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>
j-bowhay
j-bowhay reviewed Apr 9, 2025
View reviewed changes
j-bowhay
j-bowhay reviewed Apr 9, 2025
View reviewed changes
Copy link
Member
@j-bowhay j-bowhay left a comment

Choose a reason for hiding this comment

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

Thanks @swstkm, this looks basically ready just two places where it would be great to have some test coverage

@swstkm swstkm requested a review from j-bowhay April 10, 2025 10:06
j-bowhay
j-bowhay reviewed May 13, 2025
View reviewed changes
j-bowhay
j-bowhay reviewed May 13, 2025
View reviewed changes
j-bowhay
j-bowhay reviewed May 13, 2025
View reviewed changes
j-bowhay
j-bowhay reviewed May 13, 2025
View reviewed changes
j-bowhay
j-bowhay approved these changes May 14, 2025
View reviewed changes
Copy link
Member
@j-bowhay j-bowhay left a comment

Choose a reason for hiding this comment

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

Thanks all, failures look unrelated

@j-bowhay j-bowhay merged commit bd0388c into scipy:main May 14, 2025
37 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucascolley lucascolley lucascolley left review comments

@j-bowhay j-bowhay j-bowhay approved these changes

@andyfaff andyfaff Awaiting requested review from andyfaff andyfaff is a code owner

Assignees
No one assigned
Labels
enhancement A new feature or improvement scipy.optimize
Projects
None yet
Milestone
1.16.0
Development

Successfully merging this pull request may close these issues.

ENH: optimize.root: warn when inner parameters are ignored with method='krylov'
4 participants
@swstkm @janosg @lucascolley @j-bowhay
0