JuliaManifolds / Manifolds.jl

Manifolds.jl provides a library of manifolds aiming for an easy-to-use and fast implementation.
https://juliamanifolds.github.io/Manifolds.jl
MIT License
366 stars 52 forks source link

Unify use of an `atol` when comparing to zero #692

Closed kellertuer closed 8 months ago

kellertuer commented 8 months ago

As discussed therein, this fixes #630.

I carefully went through all isapprox and as soon as there was a comparison to zero, I adapted from existing cases a nonzero atol in the signature and passed that down.

this still needs a bit of checks for failing tests, but should work otherwise I hope.

kellertuer commented 8 months ago

Ah, one thing that is a bit tricky here is that without the atol= a lot of checks where quite fine with integer matrices, now they are not, maybe adopting the already sometimes used max_eps makes that nicer to tolerate those again. Most tests failing currently is due to integer values.

kellertuer commented 8 months ago

Locally the tests run fine by now – here it seems on Julia 1.10 there is a problem with DiffEq (again?).

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (b2d83b3) 99.57% compared to head (a63e8ae) 99.57%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #692 +/- ## ======================================= Coverage 99.57% 99.57% ======================================= Files 108 108 Lines 10677 10708 +31 ======================================= + Hits 10632 10663 +31 Misses 45 45 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mateuszbaran commented 8 months ago

BoundaryValueDiffEq.jl again :disappointed:

mateuszbaran commented 8 months ago

I've opened an issue: https://github.com/SciML/BoundaryValueDiffEq.jl/issues/148

kellertuer commented 8 months ago

With the last fix it really runs locally just fine (probably with an older version of BoundaryValueEq still), that is indeed a bit annoying.

mateuszbaran commented 8 months ago

@kellertuer could you check if there is anything else to adapt due to https://github.com/JuliaManifolds/ManifoldsBase.jl/pull/177 ?

kellertuer commented 8 months ago

At first glance there might be two further things (but I am on mobile)

mateuszbaran commented 8 months ago
  • the ExtrinsicEstimationMethod got a little new feature to store the method using in the extrinsic space (since usually that was only implicitly mentioned in the docs that one uses gradient in the embedding or such) – so the constructor without parameter (defaulting to GradientDescentEstimation I would think)

The most common use case for extrinsic estimation is when the embedding in Euclidean and we use EfficientEstimator there.

kellertuer commented 8 months ago

Sure that also sounds like a very good default. I think I just saw the gradient one somewhere, but yours is indeed maybe the more common case.

kellertuer commented 8 months ago

The new approximation methods and their dispatch should now be fixed. I also circumvent the inconsistency from StatsBase for now, which mainly means we have a bit of code that can be considered duplication (if StatsBase would not be inconsistent).