kalmarek / SymbolicWedderburn.jl

Amazing package to compute decompositions into irreducibles of explicit group representations and the Wedderburn decomposition for endomorphisms thereof!
MIT License
9 stars 4 forks source link

Return multiplicity info in isotypical_basis #33

Closed blegat closed 3 years ago

blegat commented 3 years ago

SumOfSquares need this info to further reduce in case the degree is not 1. Alternatively, we could return three vectors of the same size but a vector of struct seems cleaner

codecov[bot] commented 3 years ago

Codecov Report

Merging #33 (f674e1f) into master (ac5e14e) will increase coverage by 0.14%. The diff coverage is 100.00%.

:exclamation: Current head f674e1f differs from pull request most recent head b62315a. Consider uploading reports for the commit b62315a to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   89.10%   89.25%   +0.14%     
==========================================
  Files           9        9              
  Lines         505      512       +7     
==========================================
+ Hits          450      457       +7     
  Misses         55       55              
Flag Coverage Δ
unittests 89.25% <100.00%> (+0.14%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/projections.jl 86.36% <100.00%> (+1.61%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ac5e14e...b62315a. Read the comment docs.

kalmarek commented 3 years ago

very well, how about making it a subtype of AbstractMatrix?

and maybe a bit of bikeshedding: how about calling it a SemisimpleSummand? (the structure has no information of where it comes from, so isotypical can be misleading)

blegat commented 3 years ago

how about making it a subtype of AbstractMatrix?

The user should select the constituent field to get the matrix I would say. For instance, if he uses a matrix product then having this an AbstractMatrix would not allow the called to be dispatched to a method that would exploit the fact that it's a Array (e.g., by calling a blas/lapack call) and would just use a fallback for AbstractArray. So we should force the user to use .constituent.

kalmarek commented 3 years ago

yes, I understand; but I'm reluctant to expose the inner fields to the user as this is implementation detail; If someone hopes for fast BLAS routines, he'd better ask for an honest Matrix{Float64}. This way the intention is clear and explicit, and we hide the internals from the user.

What do you think?

EDIT: besides: the matrix that was returned is better named basis. constituent referred to irreducible characters which constitute the action_character (this is the representation theory jargon) ;) and this basis will never be suitable for BLAS, since it's a Matrix{Cyclotomic{T,...}}.

blegat commented 3 years ago

You mean the inner fields of SemisimpleSummand are implementation details and they might change in path release? So how does the user access the multiplicity?

kalmarek commented 3 years ago

we should probably add multiplicity function, define degree for such summand and get rid of one of the fields (multiplicity*degree == size(basis, 1))

kalmarek commented 3 years ago

@blegat I changed the generic matrix to a (slightly less ;) generic basis, since you're asking for a basis for a SemisimpleSummand; anyway: change at will, or approve ;)

kalmarek commented 3 years ago

oh wait, probably you can't approve your own pull :)

blegat commented 3 years ago

Looks good to me, let's merge and release :)