Skip to content
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

Update to Manifolds v0.10 SE(n, vectors=HybridTangentRepresentation()) #302

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

Affie
Copy link
Member

@Affie Affie commented Sep 13, 2024

Tests on master are not passing even without these changes.

@Affie Affie requested a review from dehann September 15, 2024 20:57
@Affie Affie added this to the v0.8.6 milestone Oct 3, 2024
@Affie
Copy link
Member Author

Affie commented Oct 3, 2024

I'm merging this and will sort out the current broken master branch in a separate PR

@Affie Affie marked this pull request as ready for review October 3, 2024 10:30
@Affie Affie merged commit 1899755 into master Oct 3, 2024
1 of 2 checks passed
Copy link
Member

@dehann dehann left a comment

Choose a reason for hiding this comment

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

I haven't dug into the MJL 0.10 change to the Hybrid vectors. We probably need an explicit test or the 0.10 changes.

@Affie
Copy link
Member Author

Affie commented Oct 3, 2024

For safety, all SE(n) manifolds were changed to the old behaviour (adding vectors=HybridTangentRepresentation()) so nothing changes as the old SE(n) is used throughout. The tests were passing locally in RoME and IIF, but AMP master is broken unrelated to this change.

We probably don't need vectors=HybridTangentRepresentation() everywhere, but it is a different group so has to be changed everywhere for dispatch.
As we discussed yesterday, I think the way forward is to create a "Motion(n)" (or similar name) group even if it's just an alias for SpecialEuclidean(n; vectors=HybridTangentRepresentation()) to start off with to help with this kind of problem.
We will have to be careful with depending on manifold properties/structures that is not standard as this will mean not all manifolds are supported.

@Affie Affie deleted the maint/mani_v0.10 branch October 3, 2024 11:07
@dehann dehann modified the milestones: v0.8.6, v0.9.0 Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants