Skip to content
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.

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

Only warn for early stop if options.show_trace #1049

Closed

Conversation

MilesCranmer
Copy link
Contributor

Addresses discussion in #1046 with @mohamed82008

Users of PySR and SymbolicRegression.jl were suddenly getting spammed with warning messages (e.g., MilesCranmer/PySR#416) so I had to hotfix things to use an earlier Optim.jl.

This PR addresses this so that the warning only shows up if the user requested that level of information with show_trace = true. Otherwise the warning does not show. But it will still quit early which I agree is the correct behavior.

Copy link
Contributor

@mohamed82008 mohamed82008 left a comment

Choose a reason for hiding this comment

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

The decision is Patrick's but just showing my support.

@MilesCranmer
Copy link
Contributor Author

Pinging @pkofod

@pkofod
Copy link
Member

pkofod commented Oct 7, 2023

I'm wondering if this is the right approach. If you have optimize in a loop where you don't want logging information, have you considered using with_logger(NullLogger) do .... end ?

@MilesCranmer
Copy link
Contributor Author

I don’t want to skip all logging information, just the NaNs in the Optim loop. I want all logging info from any user-specified objective, or other optimization warnings, to be displayed.

Throwing a warning for NaNs in all optimization loops is a subjective design decision. For some applications where NaNs are frequently observed (like my symbolic regression use cases), it results in a bad user experience with tons of irrelevant warnings being printed. So I’d like to be able to turn it off.

@pkofod
Copy link
Member

pkofod commented Oct 8, 2023

Okay. I personally think the options struct is way overloaded as it is, but maybe it's better to introduce a show_warnings keyword here instead? I'm not sure I agree the two are similar in nature.

@MilesCranmer
Copy link
Contributor Author

Sounds good. I didn't want to add yet another option which is why I included it in show_trace. But happy to change it.

@MilesCranmer
Copy link
Contributor Author

Let me know what your decision is?

@MilesCranmer
Copy link
Contributor Author

Ping @pkofod. What do you prefer?

@MilesCranmer
Copy link
Contributor Author

Ping @pkofod. This is holding up SymbolicRegression.jl and PySR from upgrading.

@MilesCranmer
Copy link
Contributor Author

Okay I still haven't heard any response so I'm just going to submit a second PR with show_warnings instead and you can pick the PR you want.

@MilesCranmer
Copy link
Contributor Author

Ping @mohamed82008 @pkofod. Sorry for being annoying but I've been waiting for this since August and it is still blocking me from upgrading.

@pkofod
Copy link
Member

pkofod commented Dec 12, 2023

went with #1058

@pkofod pkofod closed this Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants