Skip to content

Continue Refactoring of Landmarks inheriting from NFoldManifold and associated metrics and tests #1590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
ninamiolane merged 16 commits into from
Jun 22, 2022
Merged

Continue Refactoring of Landmarks inheriting from NFoldManifold and associated metrics and tests #1590

ninamiolane merged 16 commits into from
Jun 22, 2022

Conversation

@ninamiolane
Copy link
Collaborator

@ninamiolane ninamiolane commented Jun 21, 2022 •
edited
Loading

This PR continues to address issue #1492 by:

  • making Landmarks inherit from NFoldManifold,
  • renaming L2Metric into L2LandmarksMetric and making it inherit from NFoldMetric,
  • adapting the test cases accordingly.

The hierarchy of test data has not been adapted.

Checklist

  • My pull request has a clear and explanatory title.
  • If neccessary, my code is vectorized.
  • I have added apropriate unit tests.
  • I have made sure the code passes all unit tests. (refer to comment below)
  • My PR follows PEP8 guidelines. (refer to comment below)
  • My PR follows geomstats coding style and API.
  • My code is properly documented and I made sure the documentation renders properly. (Link)
  • I have linked to issues and PRs that are relevant to this PR.

Description

Issue

Additional context

@codecov
Copy link

codecov bot commented Jun 21, 2022 •
edited
Loading

Codecov Report

Merging #1590 (0850e4b) into master (399d33e) will decrease coverage by 3.84%.
The diff coverage is 85.72%.

@@            Coverage Diff             @@
##           master    #1590      +/-   ##
==========================================
- Coverage   90.39%   86.55%   -3.83%     
==========================================
  Files         108      103       -5     
  Lines       10364    10104     -260     
==========================================
- Hits         9367     8745     -622     
- Misses        997     1359     +362
Flag Coverage Δ
autograd ?
numpy 86.55% <85.72%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
geomstats/geometry/discrete_curves.py 78.99% <66.67%> (ø)
geomstats/geometry/product_riemannian_metric.py 90.00% <81.82%> (-1.40%) ⬇️
geomstats/geometry/landmarks.py 100.00% <100.00%> (+11.12%) ⬆️
geomstats/geometry/product_manifold.py 97.15% <100.00%> (+0.03%) ⬆️
geomstats/geometry/pullback_metric.py 33.07% <0.00%> (-54.83%) ⬇️
geomstats/learning/geodesic_regression.py 31.17% <0.00%> (-52.59%) ⬇️
geomstats/geometry/sub_riemannian_metric.py 43.40% <0.00%> (-50.94%) ⬇️
geomstats/information_geometry/gamma.py 53.34% <0.00%> (-38.09%) ⬇️
geomstats/geometry/fiber_bundle.py 62.17% <0.00%> (-29.72%) ⬇️
geomstats/_backend/_common.py 75.00% <0.00%> (-25.00%) ⬇️
... and 12 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 399d33e...0850e4b. Read the comment docs.

@ninamiolane ninamiolane marked this pull request as draft June 22, 2022 00:36
@ninamiolane
Copy link
Collaborator Author

ninamiolane commented Jun 22, 2022

The failing tests are either:

  • not related to this PR
  • FAILED tests/tests_geomstats/test_landmarks.py::TestL2LandmarksMetric::test_log_is_tangent[connection_args0-space0-point0-base_point0-0.09999999999999999
    --> this is a random occurence of the failure.

@ninamiolane ninamiolane marked this pull request as ready for review June 22, 2022 01:55
@ninamiolane
Copy link
Collaborator Author

ninamiolane commented Jun 22, 2022

Merging as this is an issue fix.

@ninamiolane ninamiolane merged commit 5edfe27 into geomstats:master Jun 22, 2022
@ninamiolane ninamiolane deleted the el-refactor branch June 22, 2022 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ninamiolane