-
-
Notifications
You must be signed in to change notification settings - Fork 100
Rewrite the progressbar part of OptimizationOptimisers
#1060
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
Conversation
9cbffa3 to
89e2d0c
Compare
|
Now, it's similar to the ODE progress logging. |
| for epoch in 1:epochs | ||
| if breakall | ||
| break | ||
| progress_id = uuid4() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about using a random UUID here. That as a symbol will intern. What's the reasoning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProgressLogging states that id should be UUID:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String or symbol? because we've never used a UUID there before, at least a string one. It at least before was a symbol, and we would get memory leaks if it was unique. So check if it's always a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it exactly is UUID-type in ProgressLogging
but in Base.CoreLogging, _id is symbol
|
Doing it the safe way but can also change in the future, this just gets it done. |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Fixes #1059