Unify use of an atol when comparing to zero

[kellertuer]

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]

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]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #692   +/-   ##
=======================================
  Coverage   99.57%   99.57%           
=======================================
  Files         108      108           
  Lines       10677    10708   +31     
=======================================
+ Hits        10632    10663   +31     
  Misses         45       45           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mateuszbaran]

BoundaryValueDiffEq.jl again 😞

[mateuszbaran]

I've opened an issue: SciML/BoundaryValueDiffEq.jl#148

[kellertuer]

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]

Everything else looks fine 👍

[mateuszbaran]

@kellertuer could you check if there is anything else to adapt due to JuliaManifolds/ManifoldsBase.jl#177 ?

[kellertuer]

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

• default_approximation_method is the new function and we have an old (deprecated) default_estimation_method now here
• 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)

[mateuszbaran]

• 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]

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]

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).

[mateuszbaran]

Cool, it looks almost ready 👍