-
Notifications
You must be signed in to change notification settings - Fork 56
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
Adapt to ManifoldsBase 1.0 #781
base: master
Are you sure you want to change the base?
Conversation
…iaManifolds/Manifolds.jl into kellertuer/adapt-to-ManifoldsBase-16
….com/JuliaManifolds/Manifolds.jl into kellertuer/adapt-to-ManifoldsBase-16
|
The same way now all others that have fused exp or retracts need to be fixed.
….com/JuliaManifolds/Manifolds.jl into kellertuer/adapt-to-ManifoldsBase-16
…ork but now they actually do work
My rework from the weekend makes all tests pass, so after releasing 1.0, we only have to check for test coverage by now. |
This should be finished ± checking for code coverage after all the changes to exp/retr. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #781 +/- ##
==========================================
+ Coverage 96.45% 96.52% +0.06%
==========================================
Files 127 127
Lines 11890 11957 +67
==========================================
+ Hits 11469 11541 +72
+ Misses 421 416 -5 ☔ View full report in Codecov by Sentry. |
Docs are already fixed here locally, checking tutorials already. But will only push after I see how the tests are doing and maybe working on code cov there already. |
Raised number of ambiguities allowed to 35 – noticed that it is also 35 on 1.11; but we exclude 1.11 from testing ambiguities; so it did not error locally for me. Do we want to keep that exclusion? Do we want to keep the ambiguity test or is the one from Aqua enough? (I just do not remember the arguments maybe). |
Maybe let's keep the exclusion; it prevents spurious failures on CI when a new Julia version is released. The bump in allowed ambiguities should be OK. The Aqua test makes wider exclusions than ours so I'd prefer to keep both. |
Ok, just means that me running on 1.11 would not see this happening and only CI would fail. But I checked the new ambiguities and they look harmless. |
We have code coverage – I even fixed a few previously uncovered lines, to slowly increase code cov here. If it is ok, I would like to merge this and register a new version. You can merge an register as well of course; for me it would be great in order to continue with the 3 PRs that depend on this. |
This depends on JuliaManifolds/ManifoldsBase.jl#221 being merged.
Overall I went through all occurrences of
exp
andexp!
and split/renamed them toexpt
andexpt!
, respectively, where necessary.There are two tricky areas left
MetricManifold
, which should carefully be checkedsolve_exp_ode
where I can not yet see why they fail because the error messages basically kill my terminal in length.Formerly this is breaking, because we drop support for ManifoldsBase 0.15?
Furthermore all occurrences of
retract
,retract_
and all its follow up functions need to be split for each and every retraction method on every manifold.TVector
toTangentvector