JuliaManifolds / ManifoldsBase.jl

Basic interface for manifolds in Julia
https://juliamanifolds.github.io/ManifoldsBase.jl/
MIT License
83 stars 8 forks source link

Unify `error=` keyword #170

Closed kellertuer closed 9 months ago

kellertuer commented 9 months ago

This resolves #135.

kellertuer commented 9 months ago

But there seem to be a few dispatches through the decorators that are quite tricky still.

kellertuer commented 9 months ago

One thing missing as well is that :info and :warn were previously only used on DomainErrors, now that they here also end up being used for ManifoldDomainErrors, the string generation fails. This I have to take a look at what best to do.

edit: For now there is a fix, but still info/warn basically only work for ErrorExceptions that have a val and a msg field. This could be improved.

codecov[bot] commented 9 months ago

Codecov Report

Merging #170 (4704c8a) into master (2d1ac2b) will decrease coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
- Coverage   99.96%   99.96%   -0.01%     
==========================================
  Files          26       26              
  Lines        3065     3063       -2     
==========================================
- Hits         3064     3062       -2     
  Misses          1        1              
Files Coverage Δ
src/ManifoldsBase.jl 99.58% <100.00%> (-0.42%) :arrow_down:
src/ValidationManifold.jl 100.00% <100.00%> (ø)
src/decorator_trait.jl 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

kellertuer commented 9 months ago

In principle this PR is finished – Docs are updated, even the tutorial (but the push to Documenter 1.0 comes from another PR, the tutorial is updated here already)

kellertuer commented 9 months ago

I just had to adapt 2 tests from the other PRs, merged the NEWS – there we only have to enter the date when we merge/register 0.15

mateuszbaran commented 9 months ago

Do we adapt tests like @test_throws DomainError is_vector(M, q, YN, true) to @test_throws DomainError is_vector(M, q, YN; error=:error) or something else?

kellertuer commented 9 months ago

Yes. I wrote that in detail in the NEWS.md, to unify all signatures I had to switch 2 positionals.

mateuszbaran commented 9 months ago

I see, sorry -- I forgot to read NEWS.md.

mateuszbaran commented 9 months ago

The CI issue is really werid...

kellertuer commented 9 months ago

Yes. It worked on the comment before so maybe let's wait a bit or check whether they did something to that package? (I am on mobile currently).

mateuszbaran commented 9 months ago

I've tried tracking it and I suspect https://github.com/JuliaRegistries/General/pull/93498 is the issue.

mateuszbaran commented 9 months ago

I've mostly updated Manifolds.jl. Could you take a look why a wrong exception type is throws in centered_matrices.jl, fixed_rank.jl and probability_simplex.jl?

kellertuer commented 9 months ago

Not today, maybe tomorrow before my lecture, otherwise only early evening (travelling to Oslo in the afternoon)

mateuszbaran commented 9 months ago

No problem, it can wait.

kellertuer commented 9 months ago

I checked them and adapted the tests – indeed they now show “X error caused by Y error” which even looks nicer than the ManifoldDomainErrors (that we still have in other places where they fit better).

mateuszbaran commented 9 months ago

Thanks!