Nemocas / AbstractAlgebra.jl

Generic abstract algebra functionality in pure Julia (no C dependencies)
https://nemocas.github.io/AbstractAlgebra.jl/dev/index.html
Other
163 stars 63 forks source link

Make GroupElem broadcastable #1714

Closed lgoettgens closed 4 months ago

lgoettgens commented 4 months ago

Replaces and thus closes https://github.com/oscar-system/Oscar.jl/pull/3787.

We already have the analogous thing for NCRingElem, so I see no reason not to have it for GroupElem.

cc @ThomasBreuer

ThomasBreuer commented 4 months ago

Thanks.

Then what about AbstractAlgebra.AdditiveGroupElem?

lgoettgens commented 4 months ago

Then what about AbstractAlgebra.AdditiveGroupElem?

There is no interface for AdditiveGroupElem defined at all here. Thus I would find it a bit weird to just add broadcastable and nothing else. Maybe @fingolfin or @thofma have some opinion on this?

thofma commented 4 months ago

This will clash with broadcasting of matrices.

ThomasBreuer commented 4 months ago

This will clash with broadcasting of matrices.

I had thought that such problems don't occur because we distinguish between matrices and matrix group elements.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.91%. Comparing base (8a5e06f) to head (16f1e1d).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1714 +/- ## ========================================== - Coverage 87.33% 86.91% -0.43% ========================================== Files 117 117 Lines 29824 29799 -25 ========================================== - Hits 26047 25899 -148 - Misses 3777 3900 +123 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

thofma commented 4 months ago

I had thought that such problems don't occur because we distinguish between matrices and matrix group elements.

This is true, but unrelated.

julia> x = ZZ[1 2; 3 4]
[1   2]
[3   4]

julia> x isa AbstractAlgebra.AdditiveGroupElem
true
lgoettgens commented 4 months ago

Thanks @thofma. So we should not add this for AdditiveGroupElem but only for FinGenAbGrpElem