-
Notifications
You must be signed in to change notification settings - Fork 64
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
feat: group algebras with sparse representation #1655
base: master
Are you sure you want to change the base?
Conversation
032ba33
to
d4cbc4f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1655 +/- ##
==========================================
- Coverage 75.85% 75.82% -0.04%
==========================================
Files 361 361
Lines 113719 113797 +78
==========================================
+ Hits 86262 86284 +22
- Misses 27457 27513 +56
|
Does this change allow me to work with group algebras of infinite groups as well? |
Yes:
|
This is wonderful! I will shift to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Some quick comments and questions (all nitpicks, not meant to block anything)
```jldoctest | ||
julia> G = abelian_group([2, 2]); a = G([0, 1]); | ||
julia> QG = group_algebra(QQ, G); | ||
julia> x = QG(a) | ||
[0, 0, 1, 0] | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bit redundant to me given the example right afterwards. But perhaps that just me (definitely is a matter of taste anyway).
```jldoctest | |
julia> G = abelian_group([2, 2]); a = G([0, 1]); | |
julia> QG = group_algebra(QQ, G); | |
julia> x = QG(a) | |
[0, 0, 1, 0] | |
``` |
Alternatively, how about turning this into a "running" example, with the codeblocks connected with help of a label? I.e. using the ```jldoctest mylabel
syntax. Then the definitions of G
and QG
don't have to be repeated three times.
julia> QG = group_algebra(QQ, G); | ||
julia> QG(Dict(a => 2, zero(G) => 1)) == 2 * QG(a) + 1 * QG(zero(G)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about also supporting this syntax?
julia> QG(Dict(a => 2, zero(G) => 1)) == 2 * QG(a) + 1 * QG(zero(G)) | |
julia> QG(a => 2, zero(G) => 1) == 2 * QG(a) + 1 * QG(zero(G)) |
@@ -59,17 +59,25 @@ end | |||
################################################################################ | |||
|
|||
@doc raw""" | |||
group_algebra(K::Ring, G; op = *) -> GroupAlgebra | |||
group_algebra(K::Ring, G::Group; sparse::Bool = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is op
omitted on purpose (just wondering, as it is still there in the implementation)
@@ -130,6 +130,13 @@ end | |||
@attributes mutable struct GroupAlgebra{T, S, R} <: AbstractAssociativeAlgebra{T} | |||
base_ring::Ring | |||
group::S | |||
# We represent elements using a coefficient vector, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# We represent elements using a coefficient vector, | |
# If `sparse` is false we represent elements using a coefficient vector, |
A.ind += 1 | ||
resize!(A.base_to_group, max(A.ind, length(A.base_to_group))) | ||
A.base_to_group[A.ind] = g | ||
return A.ind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So when would length(A.base_to_group)
ever exceed A.ind
? Right now I am wondering why you even need A.ind
and can't just do this:
A.ind += 1 | |
resize!(A.base_to_group, max(A.ind, length(A.base_to_group))) | |
A.base_to_group[A.ind] = g | |
return A.ind | |
push!(A.base_to_group, g) | |
return length(A.base_to_group) |
|
||
A.one = v | ||
if !A.sparse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge this with preceding end
into an else
?
Due to popular demand, group algebras with sparse representation (aka "don't crash with large groups"). Needs some work/clean up:
@royess this might interest you. It is similar to your approach.
Small todo:
FG(::Dict)
constructor.