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.
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.
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.
@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)
endwhich 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.
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.
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.
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.
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.
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. |
ThomasBreuer
force-pushed
the
TB_map_entries
branch
from
ebd3585
to
9a08bbc
Compare
|
Huh, doctest failures in |
See #3377 . |
|
This should get backported, right? |
Bump @fingolfin @ThomasBreuer |
fingolfin
added
the
backport 1.0.x
(cherry picked from commit eb7bbdf)
benlorenz
removed
the
backport 1.0.x
### 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.