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

More general columnwise multiplication action #756

Merged
merged 7 commits into from
Oct 20, 2024

Conversation

olivierverdier
Copy link
Contributor

This makes ColumnwiseMultiplicationAction more general, as it works now with any subtype of AbstractMatrixGroup, a new abstract type above GeneralLinear and GeneralUnitary.

Feel free to change the new name if you don't like it!

Maybe other groups could be subgroups of this new abstract type?

I also think more actions could be defined with respect to that abstract type instead of GeneralUnitary.

What do you think?

Copy link

codecov bot commented Oct 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.38%. Comparing base (41e170d) to head (66fb5c1).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #756   +/-   ##
=======================================
  Coverage   96.38%   96.38%           
=======================================
  Files         123      123           
  Lines       11438    11438           
=======================================
  Hits        11025    11025           
  Misses        413      413           

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

Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

Thanks, I think this is a good idea.

One thing I'm unsure about at the moment is that it also makes sense for SE(n) to be able to act through ColumnwiseMultiplicationAction but not in a straightforward way and we can't make special Euclidean a subtype of AbstractMatrixGroup. So maybe ColumnwiseMultiplicationAction should be even more general and allow for arbitrary On, and some groups would have to specialize apply for this action.

src/groups/general_linear.jl Outdated Show resolved Hide resolved
@olivierverdier
Copy link
Contributor Author

Yes, that's a good point, I agree with that.

Maybe something else is required for that: a diagonal action, that is, when you have an action of $G$ on a space $V$, you should be able to construct the diagonal action of $G$ on $V^n$ for some integer $n$.

Then the columnwise action on $d\times n$ matrices is essentially a diagonal action.

That would solve the issue in the most general way, I believe?

So a rough sketch of how it would look like is:

struct ColumnwiseMultiplicationAction{
    TA<:AbstractGroupAction{TAD}
} <: AbstractGroupAction{TAD}
    action::TA
end

function apply(A::ColumnwiseMultiplicationAction, a, p)
    return apply(DiagonalAction(A.action, size(p,2)), a, p) # but DiagonalAction doesn't exist yet (and not sure if `p` or `eachcol(p)`)
end

@kellertuer
Copy link
Member

kellertuer commented Oct 14, 2024

Just a first super small remark. In LieGroups.jl there is only one (concrete) type LieGroup{Field, Manifold, GroupOperation} would a MatrixLieGroup there make sense as well? Then we need AbstractLieGroup somewhen there (maybe anyways) – or is it more useful to do an AbstractMatrixMultiplicationGroupOperation since we have a MultiplicationGroupOperation already anyways?

edit: there is also no need to hurry, in the current speed and with just me writing on Lie groups. I would like to finish most groups and their operations first, so until I come to concrete actions in that speed, is probably still quite a few weeks to go.

@olivierverdier olivierverdier changed the title More genral columnwise multiplication action More general columnwise multiplication action Oct 15, 2024
@mateuszbaran
Copy link
Member

Maybe something else is required for that: a diagonal action, that is, when you have an action of

Good idea, I will think about it.

Just a first super small remark. In LieGroups.jl there is only one (concrete) type LieGroup{Field, Manifold, GroupOperation} would a MatrixLieGroup there make sense as well? Then we need AbstractLieGroup somewhen there (maybe anyways) – or is it more useful to do an AbstractMatrixMultiplicationGroupOperation since we have a MultiplicationGroupOperation already anyways?

At the moment it's not entirely clear what that abstract type would exactly mean. I think we can implement the key idea here without that type anyway, so maybe there is no need to think about implementing it in LieGroups.jl.

@olivierverdier
Copy link
Contributor Author

olivierverdier commented Oct 15, 2024

Yes, I suppose it could make sense for LieGroups.jl as well?

But I'm not 100% sure that this is the correct way to handle the issue in this case, since, as @mateuszbaran observed, multicolumn action should also be available for direct and semidirect product, and the current solution doesn't address these cases. I honestly don't know what the best solution would be.

@kellertuer
Copy link
Member

Sure, taking this as a bit of a playground and experiment sounds good. I feel it might be nicer to introduce an AbstractMultiplicationOp and a MatrixOp or so on the operations side and subtype there. But I have not yet fully thought through that and all ist implications either.

@mateuszbaran
Copy link
Member

I've switched AbstractMatrixGroup to a union type MatrixGroup for now. This should now be a simple extension that won't need too much consideration and we can think about something more general for LieGroups.jl later. Is that OK?

@olivierverdier
Copy link
Contributor Author

Yes, I think that will do, thank you.

@mateuszbaran mateuszbaran merged commit 5d28783 into JuliaManifolds:master Oct 20, 2024
17 of 19 checks passed
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.

3 participants