Hyperparameter optimization #15
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 1106 1106
=========================================
Hits 1106 1106 ☔ View full report in Codecov by Sentry. |
|
@kellertuer I'm not sure if we should actually attempt running the hyperparameter optimization on CI? It's fairly expensive but I will try to restrict it. |
|
We have a few examples and/or tutorials even that are run locally and just their Markdown checked in. For example the geodesic regression (tutorial in Manopt) which requires rendering with Asymptote, which can not be done on CI. So for any Benchmark I would do the same since it would probably run for too Lon on CI. |
|
Good idea, I will try that. |
|
This isn't going to work without JuliaManifolds/Manopt.jl#361 so we should finish that first. |
|
@kellertuer everything works now here 🙂 |
mateuszbaran
added
the
Ready-for-Review
|
Nice, I can take a closer look the next few days. At first glance:
|
|
I considered dividing it further but there is no obvious (to me) point at which it would make sense. Feel free to suggest which block to cut though. |
There was a problem hiding this comment.
Oh I added the remarks to the md file, sorry.
Sone code blocks are a bit long but I do not see a good idea how to shorten them.
One thing I would prefer to have is a short interpretation of the search run, what do the results mean here, which method to use? Something like manifold = 1, retr=3 is a bit magic.
|
|
||
| norm_inf(M::AbstractManifold, p, X) = norm(X, Inf) | ||
|
|
||
| struct TTsuggest_int |
There was a problem hiding this comment.
I am not so sure what this struct and the following are, but I suppose they are merely collecting results or values? Maybe that could be mentioned?
There was a problem hiding this comment.
They are purely technical detail for collecting reported data from a single run.
There was a problem hiding this comment.
Maybe add exactly this sentence (no phrased a bit nicer maybe) somewhere then :)
| TTreport(Float64[]), | ||
| TTshould_prune(), | ||
| ) | ||
| od(tt) |
There was a problem hiding this comment.
This line I do not understand – od is just a struct from the current definitions with quite some fields, how can it be called with one parameter?
There was a problem hiding this comment.
It has a method written later. Anyway, compute_pruning_losses is another technical detail, I will move it to another cell.
There was a problem hiding this comment.
Add at least a comment what that does here, or move it to later (yeah saw it later as well then).
Sure, I will add that. |
|
I think I've addressed all your comments but I'm not sure why the documentation preview isn't updated. Maybe we are caching too much? |
|
I don't think we cache too much, but maybe the deployment failed? Hm, does not look like. Then I am unsure why it did not update |
|
The issue with documenter job not recognizing ManifoldsBase.jl in another PR was exactly caching -- I've removed cached data and the next run succeeded. I've cleaned the cache here and re-run documenter, when it finishes we will see. |
|
I See. Caching the packages speeds things up here, but seems to sometimes cause this, then. |
|
I think this is ready to be merged now? |
I've moved the important part of JuliaManifolds/Manopt.jl#339 here.