Skip to content

Propagate product metric parallel transport to factors - #2131

Merged
luisfpereira merged 2 commits into
geomstats:mainfrom
Afloat16:fix-product-metric-parallel-transport
Oct 7, 2026
Merged

luisfpereira merged 2 commits into
geomstats:mainfrom
Afloat16:fix-product-metric-parallel-transport

Conversation

@Afloat16

@Afloat16 Afloat16 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2130, reported by @luisfpereira.

ProductRiemannianMetric inherits an unimplemented parallel_transport, so PolyHyperbolicCholeskyMetric works for 2×2 correlation matrices but fails for 3×3 and larger ones. The product connection splits across factors; delegate transport to those metrics using the existing argument projection and output packing. Factors without transport continue to raise their own unsupported-operation error.

Tests cover direction/end-point paths, flat/stacked representations, batching, factor results, tangency and norm preservation. A 3×3 correlation regression checks both paths and reverse transport.

Verification and unmet prerequisites

This draft does not meet the contributing guide's all-tests-pass prerequisite.

Exact head 22959f5045dc87b13ef4e57267d454e58598912e, Python 3.12.14 with supported dependency bounds, CPU:

  • Five new regressions pass on NumPy, Autograd and PyTorch and fail on pristine 9941df0a3 (NumPy). Four affected native modules passed 2641 tests per backend, with 49 skips.
  • Full pytest --cov=geomstats tests/ on NumPy, including slow/redundant/script nodes: 17284 passed, 26 failed, 556 skipped, 61 xfailed, 212 xpassed; every one of the 18139 collected nodes is accounted for. Six failures are LieCholeskyMetric assertions. Twenty are notebook kernel startup blockers: ZeroMQ reports Operation not permitted, followed by Kernel died before replying to kernel_info; their notebook cells were not executed. A supported escalated kernel smoke has the same failure.
  • The guide explicitly requires ≥90% coverage for new code/features. All four executable statements in the sole new production method have 100% line coverage in the full NumPy report and focused reports on all three backends. Unfiltered whole-project NumPy line coverage is 84.223%, reported separately.
  • Full Autograd execution was killed (exit 137) before completion: 6225 passed, 1 failed, 577 skipped, 31 xfailed, 150 xpassed, 1 active node interrupted and 11190 collected nodes unrun. No final full-suite coverage report was produced. Full PyTorch execution has not started and is held for resource coordination. Earlier non-slow/non-redundant runs had separate OpenHemisphere exp/log, Autograd where dtype (Fix where for autograd #2118/Fix Autograd where output type #2129), and TorchLBFGS failures on unchanged paths. Baseline evidence does not waive the full-test prerequisite.
  • Changed files are formatted; Ruff has the same four pre-existing diagnostics as pristine source. git diff --check and Sphinx -W rendering of the new method pass.

Full-project documentation, GPU and the CI Python-version matrix were not run locally.

Checklist

  • Clear title, vectorized code and appropriate unit tests
  • New code follows the project style and API
  • New method is documented and its docstring renders
  • New-code executable line coverage is ≥90%
  • Relevant issue is linked
  • All repository tests pass on applicable backends

Current CI gate

DeepSource reports failure with documentation coverage at 58.9%, below the repository threshold, and warns that its branch baseline may be inaccurate. Its nine “introduced” findings all refer to existing definitions that are AST-identical to pristine 9941df0a3; none points to the new transport or tests. The run needs maintainer baseline/gate review. GitHub Actions are also awaiting approval.

@Afloat16

Afloat16 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

At 22959f50, new-method line coverage is 100% on all three backends; the guide's ≥90% requirement explicitly applies to new code. Full NumPy tests/ accounted for all 18,139 collected nodes, with six scientific test failures and twenty notebooks blocked at native ZeroMQ kernel startup (Operation not permitted, then Kernel died before replying to kernel_info); a standard escalated smoke also failed. Pristine production 9941df0a3 with the identical final test corpus had ten scientific test failures (five expected new-regression failures and five LieCholesky failures), plus twenty blocked notebooks. The full runs had differing stochastic outcomes; their runtime random inputs were not preserved as an identical replay, so this comparison does not reproduce every original assertion on identical inputs or waive any failure. Full Autograd was killed (exit 137), leaving 11,191 collected nodes interrupted or unrun; full PyTorch has not started.

Could you advise how to fulfill the required full-suite validation under these limits? Separately, DeepSource fails at 58.9% documentation coverage and reports nine existing definitions that are AST-identical to pristine source, while warning that its branch baseline may be inaccurate. Could you advise how to investigate that baseline/coverage gate? This remains an unready draft.

@luisfpereira
luisfpereira marked this pull request as ready for review October 7, 2026 18:25
@luisfpereira
luisfpereira force-pushed the fix-product-metric-parallel-transport branch from f74c2cc to e4040ed Compare October 7, 2026 18:26

@luisfpereira luisfpereira left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Afloat16!

I've made the tests lighter (by deleting some of the additions) because the product Hypersphere(dim=3, equip=False), Hyperboloid(dim=3, equip=False) allows for parallel transport (and the current tests automatically check its properties).

@luisfpereira
luisfpereira merged commit b001bc1 into geomstats:main Oct 7, 2026
@codecov

codecov Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.50%. Comparing base (9941df0) to head (e4040ed).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2131      +/-   ##
==========================================
+ Coverage   90.38%   90.50%   +0.12%     
==========================================
  Files         171      171              
  Lines       15852    15978     +126     
==========================================
+ Hits        14327    14459     +132     
+ Misses       1525     1519       -6     
Flag Coverage Δ
autograd 87.26% <100.00%> (?)
numpy ?
pytorch 88.66% <100.00%> (+0.02%) ⬆️

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix propagation of parallel transport to factors in ProductRiemannianMetric

2 participants