Skip to content

Bump ManifoldsBase to 1.0 #55

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.

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

Merged
merged 2 commits into from
Feb 5, 2025
Merged

Bump ManifoldsBase to 1.0 #55

merged 2 commits into from
Feb 5, 2025

Conversation

kellertuer
Copy link
Member

I am not sure we can keep the 0.15, but for now I also do not understand the error messages of the tests failing. Something with Duals, and I never found the time to read up on all the dual variables magic.

@kellertuer kellertuer changed the title Bump ManiofldsBase to 1.0 Bump ManifoldsBase to 1.0 Feb 5, 2025
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.69%. Comparing base (b8c3464) to head (6a21119).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #55      +/-   ##
==========================================
+ Coverage   95.46%   95.69%   +0.22%     
==========================================
  Files          21       13       -8     
  Lines         375      395      +20     
==========================================
+ Hits          358      378      +20     
  Misses         17       17              

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

@kellertuer
Copy link
Member Author

Nice! Locally this failed with conversion errors between Float and Duals :D

@kellertuer kellertuer added preview docs Add this label if you want to see a PR-preview of the documentation Ready-for-Review labels Feb 5, 2025
@kellertuer kellertuer merged commit acd0cd6 into main Feb 5, 2025
15 checks passed
@mateuszbaran
Copy link
Member

Are you sure this ran with ManifoldsBase.jl 1.0? I can see "⌅ [3362f125] ManifoldsBase v0.15.24" in the CI logs.

@mateuszbaran
Copy link
Member

This couldn't run with 1.0 because Manifolds.jl is a test dependency and there is no registered Manifolds.jl that supports ManifoldsBase v1

@kellertuer
Copy link
Member Author

Oh, I did not see that. Can you than take a look and see what is going wrong?

I am already quite confused what would be the right order to update. For me it seems we would have to register Manifolds.jl and ManifoldDiff.jl at the same time which is not possible?

@kellertuer
Copy link
Member Author

Yes...and Manifolds.jl tests currently can not run because they require ManifoldDiff to be already bumped to a next version.

That is exactly the deadlock I had locally yesterday, So we have to resolve this circular dependency.

@mateuszbaran
Copy link
Member

IMO ManifoldDiff.jl should be first and we have to accept CI failures until Manifolds.jl is updated. But we need to be 100% sure ManifoldDiff.jl actually works with ManifoldsBase.jl 1.0.

@kellertuer
Copy link
Member Author

Could you check that then? There are two errors appearing with conversion from duals to floats or so, I do not understand them.

@mateuszbaran
Copy link
Member

OK, I will check that.

@kellertuer
Copy link
Member Author

In the long run we should try to avoid the circular dependency. I would prefer this here not to depend on Manifods.jl

@mateuszbaran
Copy link
Member

I agree. This package should not depend on Manifolds.jl for tests.

@kellertuer
Copy link
Member Author

Since tests run locally fine for 1.0 and here (missing Manifolds version) fine for the previous one as well – can we just register and then it is fine?

@mateuszbaran
Copy link
Member

Yes, it should be fine now to register a new version of ManifoldDiff.jl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview docs Add this label if you want to see a PR-preview of the documentation Ready-for-Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants