Improve Test style and coverage #319

kellertuer · 2020-12-26T13:56:48Z

This PR resolves #259 and

I checked all files and for the lines not mentioned below we do have tests that do not trigger somwhow/why.
I removed all >= 1.3 checks since we have 1.3 as a minimium requirement already.
should we exclude the manifold test functions form coverage? Currently a few @testsets are missed and a few nonmutating else-cases,

The following is a list of lines where I am not sure how to cover those, but it would be great to have these covered, too.
Sure with our current coverage this is maybe not that necessary, I just would like to have some feedback in them :)

…rims tests via @info.

…incareBallTVectors

codecov · 2020-12-26T15:06:28Z

Codecov Report

Merging #319 (4922d17) into master (a1b604f) will increase coverage by 0.88%.
The diff coverage is 86.45%.

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   95.97%   96.86%   +0.88%     
==========================================
  Files          69       69              
  Lines        4646     4655       +9     
==========================================
+ Hits         4459     4509      +50     
+ Misses        187      146      -41

Impacted Files	Coverage Δ
src/Manifolds.jl	`100.00% <ø> (ø)`
src/distributions.jl	`50.00% <ø> (+16.66%)`	⬆️
src/manifolds/CenteredMatrices.jl	`100.00% <ø> (ø)`
src/manifolds/HyperbolicHyperboloid.jl	`100.00% <ø> (+6.74%)`	⬆️
src/manifolds/ProjectiveSpace.jl	`100.00% <ø> (ø)`
src/manifolds/Sphere.jl	`100.00% <ø> (ø)`
...manifolds/SymmetricPositiveDefiniteLinearAffine.jl	`100.00% <ø> (ø)`
src/riemannian_diff.jl	`89.47% <ø> (ø)`
src/utils.jl	`83.63% <ø> (+4.68%)`	⬆️
src/tests/tests_group.jl	`94.33% <53.84%> (ø)`
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1b604f...d01401e. Read the comment docs.

mateuszbaran · 2020-12-26T22:24:33Z

Wow, thanks for working on it. That's a nice cleanup. I will try to find some time to give you some feedback here.

mateuszbaran · 2020-12-29T10:51:35Z

should we exclude the manifold test functions form coverage? Currently a few @testsets are missed and a few nonmutating else-cases,

I wouldn't exclude them but covering them seems like a low priority task.

Sure with our current coverage this is maybe not that necessary, I just would like to have some feedback in them :)

Sure, we have a good coverage but of course it can be improved 🙂 .

[ ] What do we do about the statistics default warning?

Which default warning?

[ ] Can we do a test for https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/groups/metric.jl#L216

Sure, we just need a metric group manifold with a non-bi-invariant metric.

How can we best test the det_local_metric? For example

https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/manifolds/PowerManifold.jl#L73

https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/manifolds/ProductManifold.jl#211

I think the whole metric stuff needs a larger rework aimed at addressing #6. IIRC this quantity depends on the selection of a chart so this is somewhat broken now. I think this is perhaps supposed to return this for normal coordinates which is I think well defined but I don't like it anyway.

How can we best test the Powerdistribution? https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/manifolds/PowerManifold.jl#L191

Can we test allocation promotion on Product? https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/manifolds/ProductManifold.jl#109

Can we test ProductFVectorDistribution? https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/manifolds/ProductManifold.jl#799

Can we test ProductManifolds _rand? https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/manifolds/ProductManifold.jl#861

That looks doable, I'll take a look at it later.

[ ] Which errors do appear in Rotations? https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/manifolds/Rotations.jl#L438

I don't know, sylvester for custom array types could throw something else so I wouldn't touch it.

Can we make up a sopecial case for this situation in Rotations log!? https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/manifolds/Rotations.jl#L504

Probably yes but it looks like a very special case.

I hope there is an easy way for the special 1D rotations matrix random? https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/manifolds/Rotations.jl#L662

Sure, I can add that.

kellertuer · 2020-12-29T10:59:25Z

Thanks for the long response. I think we do not have to solve all of my points, for example if the det local metric is not yet finished, there is surely no need to do a test.

[ ] What do we do about the statistics default warning?

Which default warning?

Sorry, that I forgot to link something, this one
https://github.com/JuliaManifolds/Manifolds.jl/pull/319/checks?check_run_id=1611331687#step:5:395

[ ] Which errors do appear in Rotations? https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/manifolds/Rotations.jl#L438

I don't know, sylvester for custom array types could throw something else so I wouldn't touch it.

I did mean, to we have a case where we expect the rethrow so we can check that line? Or can we do that in a one-line instead of with an if?

mateuszbaran · 2020-12-29T11:24:52Z

Thanks for the long response. I think we do not have to solve all of my points, for example if the det local metric is not yet finished, there is surely no need to do a test.

No problem, I'll go through other points too 🙂 .

Sorry, that I forgot to link something, this one
https://github.com/JuliaManifolds/Manifolds.jl/pull/319/checks?check_run_id=1611331687#step:5:395

I'd leave it for now.

I did mean, to we have a case where we expect the rethrow so we can check that line? Or can we do that in a one-line instead of with an if?

It can be either tested by providing a custom method for sylvester for some custom types. It can also be rewritten in one line using the ternary operator ? : but the rethrow still wouldn't be covered. Ideally JuliaLang/julia#7026 would solve this.

mateuszbaran · 2021-01-02T22:27:47Z

from the utils: Are ziptuples, eiogvals (for static arrays) and eigen_safe for a StaticMatrix still needed or are they now part of `StaticArrays in some form? https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/utils.jl#L54

eigvals from StaticArrays.jl should be good enough now, I've removed it. ziptuples is still needed since it's not in StaticArrays.jl I think? eigen_safe can probably be removed as well.

[ ] is there an easy test for projected point distributions support ? https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/projected_distribution.jl#L45

There seems to be an actual major problem with projected point distributions (they seem to not work at all).

mateuszbaran · 2021-01-02T22:30:50Z

[ ] How far is the Riemannian gradient function stuff, we could add differential! and gradient! tests? https://codecov.io/gh/JuliaManifolds/Manifolds.jl/src/master/src/riemannian_diff.jl#L27

These are just some fallbacks that aren't used. Riemannian gradients should work, let me know if there are any problems. You should be able to use them in Manopt.jl.

kellertuer · 2021-01-03T11:21:37Z

Cool, I hopefully find time to check it or write an example about that then.

mateuszbaran · 2021-01-03T17:59:29Z

I've updated formatting and hopefully fixed the projected distributions. I don't know why they had the decorator dispatch but it seems to only break things. Maybe it would be nice to fix it but :parent dispatch on metric manifold, which I thought should do the right thing, apparently unwrapped the metric decorator in the Euclidean manifold tests.

kellertuer · 2021-01-03T18:02:32Z

Thanks for working on that. I hopefully find some time this week to also add a test or two. You can also mark things that we discussed.

Are there tests available for the fallbacks of the Riemannian Gradient or do we leave those untested?

mateuszbaran · 2021-01-03T18:15:52Z

Thanks for working on that. I hopefully find some time this week to also add a test or two. You can also mark things that we discussed.

Thanks, I've marked a few things but some are only partially resolved.

Are there tests available for the fallbacks of the Riemannian Gradient or do we leave those untested?

I'd vote for merging this PR before any other new work since it touches quite a few files, and any remaining points could be put in an issue. Actually, this PR may be worth splitting into a few PRs with a smaller scope instead of solving here every issue you've listed in the first comment.

kellertuer · 2021-01-03T18:19:11Z

Ok, sure, we can just mark them atop that we will do them later and finish this PR. I just came across making that list since I had rewritten our tests a little bit and an hour left to go through uncovered lines.

such that the Array variant was never used and never checked/compared.

…e the loop)

kellertuer · 2021-01-07T19:43:10Z

I think I just did 4 more lines. Can we mark the ones we move in the initial comment (maybe with an X after the box or an -> new PR at the end?) and check the box upfront?

src/statistics.jl

mateuszbaran · 2021-01-07T19:47:02Z

I think I just did 4 more lines. Can we mark the ones we move in the initial comment (maybe with an X after the box or an -> new PR at the end?) and check the box upfront?

Maybe let's merge this PR and copy all points that are not checked as done?

kellertuer · 2021-01-07T19:48:14Z

Ok. Would you mind copying those and whereto? An Issue or multiple issues?

edit: I just lost track which ones we would resolve where, that's why I am asking.

mateuszbaran · 2021-01-07T20:33:37Z

Sure, I'll open a new issue.

…op before the loop)" This reverts commit 6e71c6c.

mateuszbaran · 2021-01-08T12:15:47Z

OK, so can we merge this now?

kellertuer · 2021-01-08T12:49:13Z

In my opinion, yes.

kellertuer added 6 commits December 26, 2020 12:36

refactor runtests to work ina single testset and still print the inte…

9c6c9e5

…rims tests via @info.

increase test covaerage and resolve an error for projection on the Po…

9df4821

…incareBallTVectors

Increase test coverage further.

517e0b2

runs formatter.

fafb939

adds another small test for utils.

5d16e03

Fix a test on group general

252d4df

kellertuer added help wanted Extra attention is needed restructure labels Dec 26, 2020

mateuszbaran added 2 commits December 29, 2020 17:26

trying to cover two things

75cf10d

removing eigvals and some old version tests

11a098e

updates for distributions and formatting

a244aef

mateuszbaran and others added 7 commits January 3, 2021 22:28

add support of NormalRotationDistribution

4be37ea

metric test updates

5e25c0a

run formatter

b24b581

remove one more version check

a338809

typo in docs

d7c1aab

fixes a small error that always compared SArrays to SArrays

118c4a8

such that the Array variant was never used and never checked/compared.

removes a line that is never reached (note the injectivity atop befor…

6e71c6c

…e the loop)

mateuszbaran reviewed Jan 7, 2021

View reviewed changes

src/statistics.jl Outdated Show resolved Hide resolved

mateuszbaran mentioned this pull request Jan 7, 2021

Improve Test style and coverage -- part 2 #321

Open

12 tasks

Revert "removes a line that is never reached (note the injectivity at…

d01401e

…op before the loop)" This reverts commit 6e71c6c.

mateuszbaran merged commit 85e3940 into master Jan 8, 2021

kellertuer deleted the kellertuer/improve-test branch June 4, 2021 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Test style and coverage #319

Improve Test style and coverage #319

kellertuer commented Dec 26, 2020 •

edited

Loading

codecov bot commented Dec 26, 2020 •

edited

Loading

mateuszbaran commented Dec 26, 2020

mateuszbaran commented Dec 29, 2020

kellertuer commented Dec 29, 2020

mateuszbaran commented Dec 29, 2020

mateuszbaran commented Jan 2, 2021

mateuszbaran commented Jan 2, 2021

kellertuer commented Jan 3, 2021

mateuszbaran commented Jan 3, 2021

kellertuer commented Jan 3, 2021

mateuszbaran commented Jan 3, 2021

kellertuer commented Jan 3, 2021

kellertuer commented Jan 7, 2021

mateuszbaran commented Jan 7, 2021

kellertuer commented Jan 7, 2021 •

edited

Loading

mateuszbaran commented Jan 7, 2021

mateuszbaran commented Jan 8, 2021

kellertuer commented Jan 8, 2021

Improve Test style and coverage #319

Improve Test style and coverage #319

Conversation

kellertuer commented Dec 26, 2020 • edited Loading

codecov bot commented Dec 26, 2020 • edited Loading

Codecov Report

mateuszbaran commented Dec 26, 2020

mateuszbaran commented Dec 29, 2020

kellertuer commented Dec 29, 2020

mateuszbaran commented Dec 29, 2020

mateuszbaran commented Jan 2, 2021

mateuszbaran commented Jan 2, 2021

kellertuer commented Jan 3, 2021

mateuszbaran commented Jan 3, 2021

kellertuer commented Jan 3, 2021

mateuszbaran commented Jan 3, 2021

kellertuer commented Jan 3, 2021

kellertuer commented Jan 7, 2021

mateuszbaran commented Jan 7, 2021

kellertuer commented Jan 7, 2021 • edited Loading

mateuszbaran commented Jan 7, 2021

mateuszbaran commented Jan 8, 2021

kellertuer commented Jan 8, 2021

kellertuer commented Dec 26, 2020 •

edited

Loading

codecov bot commented Dec 26, 2020 •

edited

Loading

kellertuer commented Jan 7, 2021 •

edited

Loading