search

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
367 stars 52 forks source link

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

Closed sjacobsson closed 9 months ago

sjacobsson commented 9 months ago

…ric.

See issue #684

codecov[bot] commented 9 months ago

Codecov Report

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

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

Additional details and impacted files ```diff @@ 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 ```

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

kellertuer commented 9 months ago

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

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 https://github.com/JuliaManifolds/Manifolds.jl/blob/5d267d532036399ed9d0e31d19345dee10b64bdb/src/manifolds/CholeskySpace.jl#L6 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 :)

sjacobsson commented 9 months ago

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 commented 9 months ago

I can push changes to your pull request :slightly_smiling_face:

sjacobsson commented 9 months ago

Sure :)

kellertuer commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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