Add point capability to _produce_type to improve GPU compatibility#577
Add point capability to _produce_type to improve GPU compatibility#577mateuszbaran wants to merge 7 commits intomasterfrom
_produce_type to improve GPU compatibility#577Conversation
|
Does this test: intentionally verify that the solver works with an immutablep? I can make it work, though it would make the code a bit uglier.
|
Yes, that test is intentionally, since |
| function MomentumGradientRule(M::AbstractManifold, p; kwargs...) | ||
| return MomentumGradientRule(M; p = allocate(p), kwargs...) | ||
| end |
There was a problem hiding this comment.
I think this is now a bit inconsistent. For the DistanceOverGradient you did a breaking change and moved the p from a keyword to a positional. Here you did not do that, but introduce a new constructor with p that moves that to a keyword.
I think until now, if needed, the p was a keyword (with rand(M) as default which has the same problem with a point type by the way). To be nonbreaking, maybe keeping that would be nice?
There was a problem hiding this comment.
I understood the docs of DistanceOverGradient where they claim that p is the "initial point, used to track distance" as "p is the starting point of optimization". Is this just supposed to be an arbitrary point? If so, maybe it would be good to clarify it in the docstring. That's why I thought it's a bugfix rather than a breaking change.
OK, then I think a new method in ManifoldsBase would be the cleanest solution. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #577 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 91 91
Lines 9975 10013 +38
=========================================
+ Hits 9975 10013 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This is a draft of the enhancement discussed in #574 .
cc @zazabap