is_flat returns true for Cholesky space and SPD with log-Cholesky met…

[sjacobsson]

…ric.

See issue #684

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5d267d5) 99.57% compared to head (2299fc6) 99.53%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #685      +/-   ##
==========================================
- Coverage   99.57%   99.53%   -0.05%     
==========================================
  Files         108      108              
  Lines       10678    10678              
==========================================
- Hits        10633    10628       -5     
- Misses         45       50       +5     

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

[kellertuer]

Good morning and well done – the git and GitHub parts worked super well :)

For the CI (the sometimes a bit annoying part, but nonetheless quite important to keep our code well-maintained):

Tests

Currently the tests fail – we write tests for nearly all lines of code (>99% of our lines are covered) – so were the two lines you changed, as you can see

• here https://github.com/JuliaManifolds/Manifolds.jl/actions/runs/6855569192/job/18640887683?pr=685#step:6:1452
• and here https://github.com/JuliaManifolds/Manifolds.jl/actions/runs/6855569192/job/18640887683?pr=685#step:6:1302

both even tell you the file and the line that fails and has to be adapted. You can even run just these tests locally since all our test files are standalone runnable by just including them.

Documentation

We should document the answers more properly with references why values are returned as they are. We have documenter citations for that (you should see them for example in this line

Manifolds.jl/src/manifolds/CholeskySpace.jl

Line 6 in 5d267d5

are for example summarized in Table 1 of [Lin:2019](@cite).

how they work. Could you add the reason for the new return value similarly in the doc strings of the functions you changed? All references are in this file https://github.com/JuliaManifolds/Manifolds.jl/blob/master/docs/src/references.bib so feel freee to add papers (or look them up) for referencing

Changelog

Finally we keep track of things we changed in the NEWS.md file in the main directory (that is the other test failing), please add an entry there – and also update the version number in the Project toml accordingly.

Thanks for helping out :)

[mateuszbaran]

Thank you for the fix! I can help with failing tests if you have problems fixing them.

[sjacobsson]

Thank you for the fix! I can help with failing tests if you have problems fixing them.

That would be much appreciated I think. I don't know where to fix that.

Could you add the reason for the new return value similarly in the doc strings of the functions you changed?

we keep track of things we changed in the NEWS.md file in the main directory (that is the other test failing), please add an entry there – and also update the version number in the Project toml accordingly.

Okay, does this require a new pull request then?

[mateuszbaran]

I can push changes to your pull request 🙂

[sjacobsson]

Sure :)

[kellertuer]

to answer the new PR part: No you just need to push to the same branch you were working on, then PRs are automatically updated :)

[kellertuer]

Before this was merged I forgot to write: Thanks for both reporting and fixing this!

All contributions are always welcome and important to the package :)

[sjacobsson]

Before this was merged I forgot to write: Thanks for both reporting and fixing this!

All contributions are always welcome and important to the package :)

Thanks Ronny! I am aiming to contribute more things in the futute.

[kellertuer]

That is great to hear! Looking forward to your next contributions then - and of course we help with details similar to we did here :)