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

Introduce Kendall's shape space #550

Merged
merged 17 commits into from
Nov 12, 2022
Merged

Conversation

mateuszbaran
Copy link
Member

I've started working on Kendall's shape space (so this resolves #504 ). I'm not sure how much I'm going to cover here but at least I will make a small example of usage (maybe as a Pluto notebook if @kellertuer could help 😃 ).

@mateuszbaran mateuszbaran mentioned this pull request Nov 7, 2022
@kellertuer
Copy link
Member

Interesting!
I just decided with Brynjulf and a master student we supervise together to look into applications with Kendalls shape space to check how to maybe implement that in the future. So get this one as far as you like, she might continue from there.

@kellertuer
Copy link
Member

kellertuer commented Nov 7, 2022

Since I saw that often, we should also cover the PreShapeSpace.

And is maybe ShapeSpace too generic? There might be a lot of different shape spaces such that this might be more like a good common abstract thing?

@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #550 (597235b) into master (8fc0322) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #550      +/-   ##
==========================================
+ Coverage   98.93%   98.94%   +0.01%     
==========================================
  Files         100      102       +2     
  Lines        9664     9767     +103     
==========================================
+ Hits         9561     9664     +103     
  Misses        103      103              
Impacted Files Coverage Δ
src/Manifolds.jl 100.00% <ø> (ø)
src/differentiation/ode_callback.jl 100.00% <100.00%> (ø)
src/groups/rotation_action.jl 100.00% <100.00%> (ø)
src/manifolds/KendallsPreShapeSpace.jl 100.00% <100.00%> (ø)
src/manifolds/KendallsShapeSpace.jl 100.00% <100.00%> (ø)
src/manifolds/Sphere.jl 100.00% <100.00%> (ø)
src/tests/tests_general.jl 96.90% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mateuszbaran
Copy link
Member Author

Interesting! I just decided with Brynjulf and a master student we supervise together to look into applications with Kendalls shape space to check how to maybe implement that in the future. So get this one as far as you like, she might continue from there.

Cool! I would be nice if she could make an example using Manifolds.jl 🙂 .

Since I saw that often, we should also cover the PreShapeSpace.

That's just a special case of ArraySphere so I was going to just write in documentation that users should use that when they need preshape space.

And is maybe ShapeSpace too generic? There might be a lot of different shape spaces such that this might be more like a good common abstract thing?

That's indeed a problem, there are many variants depending on the group that is acting (here are some examples: https://digitalcommons.wayne.edu/cgi/viewcontent.cgi?article=1417&context=oa_dissertations ). I doubt, though, that we need any abstraction except quotient manifolds for them. At least I'm not going to add any in this PR. Feel free to suggest a different name, I'm not particularly convinced ShapeSpace is the right choice but haven't come up with anything better, maybe something like KendallShapeSpace?

@kellertuer
Copy link
Member

Introducing a new name or even type (that could have ArraySphare as Fallback with ExplicitDecorator) for the KendallsPreShapeSpace would be nice I think, also to have a few functions already on that level before “Quotienting out” rotations.

I would maybe go for KendallsShapeSpace since usually one has the genitive in the name, but yes tI think that would be better.

Concerning the master student – we will see. We just decided last week to go into that direction, so I will read a bit about that. I hope she will end up either contributing a few functions or a nice example/Tutorial. We will see.

@mateuszbaran
Copy link
Member Author

Introducing a new name or even type (that could have ArraySphare as Fallback with ExplicitDecorator) for the KendallsPreShapeSpace would be nice I think, also to have a few functions already on that level before “Quotienting out” rotations.

OK, I think it would be easiest to make it a new KendallsPreShapeSpace <: AbstractSphere then.

I would maybe go for KendallsShapeSpace since usually one has the genitive in the name, but yes tI think that would be better.

That sounds fine.

@mateuszbaran mateuszbaran added the preview docs Add this label if you want to see a PR-preview of the documentation label Nov 8, 2022
@mateuszbaran
Copy link
Member Author

Sorry for a small mistake, preshape space is actually a submanifold of ArraySphere, I've fixed it in the recent commit.

@kellertuer
Copy link
Member

But then even better to have it as its own type as I proposed :)

@mateuszbaran
Copy link
Member Author

Yes, exactly, it's definitely necessary 🙂 .

@mateuszbaran
Copy link
Member Author

I've rewritten a part of https://geomstats.github.io/notebooks/14_real_world_applications__hand_poses_analysis_in_kendall_shape_space.html in Manifolds.jl 🙂 .

@kellertuer
Copy link
Member

That sounds very interesting, I was a little ill a few days last week so I now am a little stressed to get back on (semester-)track. But I'll take a look as soon as I find time. Great that the PR directly also includes an example :)

I further hope the Quotient manifolds can show its potential here.

@mateuszbaran
Copy link
Member Author

No problem, it will take me a few more days to finish this so you can focus on teaching.

Previous quotient manifold work really simplified the implementation here.

@kellertuer
Copy link
Member

That was what I was hoping for, since we put quite some thought into the quotient manifold functions.

Copy link
Member

@kellertuer kellertuer left a comment

Choose a reason for hiding this comment

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

Here's a few comments.

src/groups/rotation_action.jl Show resolved Hide resolved
src/manifolds/ShapeSpace.jl Outdated Show resolved Hide resolved
src/manifolds/ShapeSpace.jl Outdated Show resolved Hide resolved
@mateuszbaran mateuszbaran mentioned this pull request Nov 9, 2022
3 tasks
@mateuszbaran mateuszbaran added extend manifold This issue proposes/asks for new functions to extend an existing manifold and removed extend manifold This issue proposes/asks for new functions to extend an existing manifold labels Nov 9, 2022
@mateuszbaran mateuszbaran added the Ready-for-Review A label for pull requests that are feature-ready label Nov 9, 2022
@mateuszbaran
Copy link
Member Author

I've looked at other features on shape spaces we could consider but they are complicated and not really essential so I'd prefer to merge this PR without them. #551 tracks those things.

@kellertuer
Copy link
Member

Could we collect the others you found in there as well?

Besides that – I hope to find some time to review this PR soon then.

@mateuszbaran
Copy link
Member Author

That issue already has the most important things. Geomstats also has a bunch of curvature derivatives I don't understand and I'm not sure what exactly they are for but that's something more general than Kendall shape spaces.

Copy link
Member

@kellertuer kellertuer left a comment

Choose a reason for hiding this comment

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

Here's a few comments.

In general it looks very nice, though I do not have an overview which other functions are available as well. I for example would love a tangent space basis (also the diagonalising one including curvature), if that exists. Maybe its even inherited from ArraySphere?

docs/src/manifolds/shapespace.md Show resolved Hide resolved
src/Manifolds.jl Outdated Show resolved Hide resolved
src/groups/rotation_action.jl Show resolved Hide resolved
src/manifolds/ShapeSpace.jl Outdated Show resolved Hide resolved
src/manifolds/ShapeSpace.jl Outdated Show resolved Hide resolved
src/manifolds/ShapeSpace.jl Outdated Show resolved Hide resolved
src/manifolds/ShapeSpace.jl Outdated Show resolved Hide resolved
src/manifolds/ShapeSpace.jl Outdated Show resolved Hide resolved
src/manifolds/ShapeSpace.jl Outdated Show resolved Hide resolved
@mateuszbaran
Copy link
Member Author

I for example would love a tangent space basis (also the diagonalising one including curvature), if that exists. Maybe its even inherited from ArraySphere?

For the pre-shape space basically everything can be worked out because it's just a fancy sphere. The only issue is that it can't reuse code for ArraySphere because of an additional constraint, which is which doesn't cause any issues with exp/log/PT but it does for bases. I'm just not sure how much work it would be.

For the shape space bases are much more complicated and I don't really want to work on it.

@mateuszbaran mateuszbaran merged commit 9366c53 into master Nov 12, 2022
@kellertuer kellertuer deleted the mbaran/landmark-manifold branch May 4, 2024 17:34
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 A label for pull requests that are feature-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kendall Shape Space
2 participants