-
-
Notifications
You must be signed in to change notification settings - Fork 98
Add output struct #101
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
Add output struct #101
Conversation
I tried to add a custom output struct that would fit the output of the respective optimizers. Some choices are probably less than perfect and several are quite arbitrary - comments more than welcome.
I didn't notice that the default value of maxiters had changed.
src/solve.jl
Outdated
@@ -1,3 +1,32 @@ | |||
abstract type OptimizationResults end #experimental; comments welcome | |||
mutable struct GalacticOptimizationResults{O, Tx, Tf, Tls, Tsb} <: OptimizationResults |
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.
Make it singular?
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.
To match the naming of our other things, I think we'd do OptimizationSolution
. Then down the line (not right now, need to do a few things), make it a AbstractNoTimeSolution
where the minimizer is .u
.
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.
Thanks for the review. I've corrected it now but there seems to be a problem with the tests. I've checked everything before submitting and it works fine. The failure trace complains about maxiters
in the CMAES test. I tested all the CMAE algorithms manually and they give a normal output. Could it be that the test itself is faulty?
@mkg33 what's the status here? |
Sorry @Vaibhavdixit02 , I was convinced I'd already updated it. Will do so immediately! Thanks for the review. |
Let's try to get tests passing and this merged. We can keep improving, but I think it's already an improvement. |
https://github.com/SciML/GalacticOptim.jl/pull/101/files#diff-ca1c45d4f0bec64e8340981f6b27f4037bf6ac13c8f1af132656c485c0a8e941L660 Evolutionary's result needs to be converted to use the struct as well @mkg33 |
@Vaibhavdixit02 I just added Evolutionary's results to the struct. Now I need to figure out why it's still failing... |
Another failure |
OK, I've analyzed the results and here is what I think: First error:
This is SAMIN but the output struct has nothing to do with it. It uses the native Optim output anyway. I'm not sure if my changes could influence the solution (?) Perhaps I just can't see it properly. So I just ran the (rosenbrock.jl) test locally and got:
|
is there any randomness there? |
That seems to be the case... I keep getting pretty varying results whenever I run the tests. So far I've had probably 10-15 different values. As in, really different, not just a matter of fractions. |
As regards the second error, it was the package resolver. I'm not sure if I had anything to do with this (?) I really don't know if outdated local versions (if they exist) might negatively influence GalacticOptim.jl. |
That's not you. That's @DhairyaLGandhi needing to tag Flux for Reexport 1.0 |
Phew :-) |
Codecov Report
@@ Coverage Diff @@
## master #101 +/- ##
==========================================
- Coverage 87.25% 84.23% -3.02%
==========================================
Files 3 3
Lines 361 387 +26
==========================================
+ Hits 315 326 +11
- Misses 46 61 +15
Continue to review full report at Codecov.
|
mutable struct OptimizationSolution{O, Tx, Tf, Tls, Tsb} <: AbstractOptimizationSolution | ||
method::O | ||
initial_x::Tx | ||
minimizer::Tx |
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.
we should have a getproperty for this to be .u
to match the others.
I tried to add a custom output struct that would fit the output of the respective optimizers. Some choices are probably less than perfect and several are quite arbitrary - comments more than welcome!