Add some more MatElem/MatRingElem conversions #1839
Conversation
lgoettgens
force-pushed
the
lg/mat-matring
branch
from
2236ba6
to
95b38b8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1839 +/- ##
==========================================
+ Coverage 88.13% 88.15% +0.01%
==========================================
Files 119 119
Lines 29982 30017 +35
==========================================
+ Hits 26426 26460 +34
- Misses 3556 3557 +1 ☔ View full report in Codecov by Sentry. |
src/Matrix.jl
Outdated
| R = base_ring(s) | ||
| for i = 1:nrows(s) | ||
| for j = 1:ncols(s) | ||
| M[i, j] = R(a[i, j]) |
There was a problem hiding this comment.
This is somewhat inefficient if base_ring(a) == base_ring(s) (as we perform a redundant parent check for every element). But we can always improve that in a future update if we deem it necessary.
There was a problem hiding this comment.
Ah, I see the point. I'll do a small improvement tomorrow
There was a problem hiding this comment.
From my POV it would be fine to merge this as-is and then tweak it later, whenever you (or someone else) has time resp. need for it. Your choice.
There was a problem hiding this comment.
done in 09933d8 (#1839). please merge if you are happy with it
| @@ -148,21 +148,7 @@ end | |||
| # | |||
| ############################################################################### | |||
|
|
|||
| # create a zero matrix | |||
| function (a::Generic.MatSpace{T})() where {T <: NCRingElement} | |||
There was a problem hiding this comment.
These days Generic.MatSpace is the only subtype of MatSpace so perhaps we should just merge the two
As discussed on slack.
The added tests demonstrate all of the new conversions that are now possible.