-
Notifications
You must be signed in to change notification settings - Fork 125
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
add map_entries
for matrix groups
#3341
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3341 +/- ##
==========================================
- Coverage 82.09% 82.07% -0.02%
==========================================
Files 556 556
Lines 74070 74226 +156
==========================================
+ Hits 60805 60922 +117
- Misses 13265 13304 +39
|
3 | ||
``` | ||
""" | ||
function map_entries(R::Ring, G::MatrixGroup) |
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.
There is also change_base_ring
for this case, which does the same. Maybe one should call the other?
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.
I think one could just have one method of the form
map_entries(f, G::MatrixGroup)
without specifying the type of the first argument. This is how all other map_*
methods usually are implemented.
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.
@joschmitt change_base_ring
seems to only cover the case where the first argument is a ring, right? Of course we can make sure one is implemented in terms of the other. The current method is
function change_base_ring(R::Ring, G::MatrixGroup)
g = dense_matrix_type(R)[]
for h in gens(G)
push!(g, map_entries(R, h.elm))
end
return matrix_group(g)
end
which does not deal with empty generator lists. We could just change it to
change_base_ring(R::Ring, G::MatrixGroup) = map_entries(R, G)
@thofma those other map_entries
methods don't need to deal with an empty list of matrices, though... But yeah one could merge the methods though one could consider the two simple methods as "optimizations" of the generic Function
one? Is there any harm in having these additional methods?
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.
No, you are right. Maybe my suggestion is just to remove the restriction on the first argument and shuffle the docstring, so that it is not at the "wrong" method (at the moment the signature of the docstring and the function are not the same).
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 could just change it to
change_base_ring(R::Ring, G::MatrixGroup) = map_entries(R, G)
Yes, exactly.
src/Groups/matrices/MatGrp.jl
Outdated
function map_entries(f::Function, G::MatrixGroup) | ||
Ggens = gens(G) | ||
if length(Ggens) == 0 | ||
o = map_entries(f, matrix(one(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.
Wouldn't it suffice to map a single entry, like
o = map_entries(f, matrix(one(G))) | |
o = f(zero(base_ring(G))) |
src/Groups/matrices/MatGrp.jl
Outdated
return matrix_group(codomain(mp), degree(G), imgs) | ||
end | ||
|
||
function map_entries(f::Function, G::MatrixGroup) |
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.
Maybe just drop the ::Function
to make it more generic "for free" ? (There are other "callable" objects which are not functions, after all)
function map_entries(f::Function, G::MatrixGroup) | |
function map_entries(f, G::MatrixGroup) |
src/Groups/matrices/MatGrp.jl
Outdated
Ggens = gens(G) | ||
if length(Ggens) == 0 | ||
o = map_entries(f, matrix(one(G))) | ||
return matrix_group(base_ring(o), degree(G), typeof(o)[]) |
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.
After discussing this: I overlooked the use of typeof(o)
here. But we can avoid it:
return matrix_group(base_ring(o), degree(G), typeof(o)[]) | |
return matrix_group(base_ring(o), degree(G), MatrixGroupElem[]) |
@ThomasBreuer it shouldn't be much work to finish this up, I hope? or are there blockers / decisions to be made? It would be nice to have this merged so I can use it. |
ebd3585
to
9a08bbc
Compare
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.
Thanks!
Huh, doctest failures in |
See #3377 . |
This should get backported, right? |
Bump @fingolfin @ThomasBreuer |
(cherry picked from commit eb7bbdf)
### Backported PRs - [x] #3367 - [x] #3379 - [x] #3369 - [x] #3291 - [x] #3325 - [x] #3350 - [x] #3351 - [x] #3365 - [x] #3366 - [x] #3382 - [x] #3373 - [x] #3341 - [x] #3346 - [x] #3381 - [x] #3385 - [x] #3387 - [x] #3398 - [x] #3399 - [x] #3374 - [x] #3406 - [x] #2823 - [x] #3298 - [x] #3386 - [x] #3412 - [x] #3392 - [x] #3415 - [x] #3394 - [x] #3391
No description provided.