kellertuer
commented
3 years ago Thanks for the thorough read and these nice comments. I will quote all comments here, marks those as finished, that I addressed/fixed and add a few comments as well.
- [x] 10: "Nelder Mead" -> "Nelder-Mead"
- [x] 11: "quasi Newton" -> "Quasi-Newton"
- [x] 41: possibly missing an "and" in "framework can"
Since the sentence was a little long already, I opted for a “. They” instead of an “and”
- [x] 55: missing comma after "manifold"
- [x] 57: maybe mention an example of a manifold with no closed forms in parentheses
this might be a little complicated. First, it depends on the metric on chooses on a manifold, and second, I think the SphereSymmetricMatrices might be en example; I will have to check literature here.
- [x] 57: "more generally arbitrary retractions" -> "more general arbitrary retractions"
- [x] 59: maybe rephrase; it seems there are two reasons for the use of retractions: closed form exponential maps are a) unavailable, or b) costly to compute; thus, it may be odd to state that these approximate algorithms are "more efficient" (than what?) especially in cases when, as stated in the preceding sentence, the closed form exponential map may not even be available
I changed “This yields approximate algorithms that are numerically more efficient.” to “Retractions are first order approximations for the exponential map. They provide an alternative to the acceleration free form, if no closed form solution is known. Otherwise, a retraction might also be chosen, when their evaluation is computationally cheaper than to use the exponential map, especially if their approximation error can be stated, see e.g. [@BendokatZimmermann:2021].”
and added the reference.
- [x] 72ff: it may be helpful to complement each item by mentioning the corresponding names in Manopt.jl explicitly, e.g. one could mention
conjugate_gradient_descentandcoefficient=DirectionUpdateRule(SteepestDirectionUpdateRule,ConjugateDescentCoefficient,DaiYuanCoefficient,FletcherReevesCoefficient,HagerZhangCoefficient,HeestenesStiefelCoefficient,LiuStoreyCoefficient, andPolakRibiereCoefficient) somewhat mimicking the nice layout of the online documentation and providing readers already a more concrete glimpse into Manopt.jl's scope and potential
Sounds like a very good idea, I will carefully check that and update the list - maybe even linking the names in Manopt.jl to the docs. Thanks for the feedback on the online documentation!
- [x] 93: typo, redundant "the"
- [x] 95: for readers less familiar with optimization on manifolds, it may be more instructive to drastically reduce
nand show the Karcher mean in contrast to the Euclidean mean so as to highlight that the Karcher mean still sits on the sphere (e.g. "a meeting half-way between Trondheim and Lausanne would take place in and not under central Europe" :-))?
This is a nice idea, I will reduce the number of points. While it might not be visually pleasing to plot the Euclidean mean, comparing the norm of the Euclidean and the Karcher mean is a very good addendum here. I just need a day or two to carefully revise the experiment.
- [x] 109ff: can the print output precision be meaningfully adjusted or aligned (the latter would, for example, help to visually identify the changes in
F(x))
This would require a little bit of rework in the debug system. Currently :Change or :Cost generates parts of the debug string where just interpolation (that is " Text $variable text" puts the variable into a string, which is called interpolation). If I would change this to some format, I can not use just a Symbol anymore but have to find a way to specify a format. Even if I map the Symbol still to some default (then fixed length) format, I would still have to redesign the DebugX functors. See for example DebugChange, where (a) the struct would have to get a field to store the print format (maybe some fprint style?) and line 144ff (where the string is created) would have to be adapted.
This is quite a nice idea and the modularity would make that possible, but it is still quite some work, mainly in the interface (i.e. how the second line in the code block after line 109 would have to look like) and a little bit in the types behind that. this, though, would take some time to be designed and realised. We can open an issue here to keep track but postpone it for later, if that would be ok?
- [x] 122: "There are" -> "The"
- [x] 144: "Bačák, M. (2014)" appears to be a duplicate of 142
Excluding the last 4 non-finished changes, the rest is done in commit 7e9334f.
sweichwald
Comments on software paper as per 6f74fee as part of https://github.com/openjournals/joss-reviews/issues/3866#event-5537686057.
Only minor comments:
conjugate_gradient_descentandcoefficient=DirectionUpdateRule(SteepestDirectionUpdateRule,ConjugateDescentCoefficient,DaiYuanCoefficient,FletcherReevesCoefficient,HagerZhangCoefficient,HeestenesStiefelCoefficient,LiuStoreyCoefficient, andPolakRibiereCoefficient) somewhat mimicking the nice layout of the online documentation and providing readers already a more concrete glimpse into Manopt.jl's scope and potentialnand show the Karcher mean in contrast to the Euclidean mean so as to highlight that the Karcher mean still sits on the sphere (e.g. "a meeting half-way between Trondheim and Lausanne would take place in and not under central Europe" :-))?F(x))