AbstractAlgebra.jl icon indicating copy to clipboard operation
AbstractAlgebra.jl copied to clipboard

Add `Base.vec(M::MatElem)`

Open lgoettgens opened this issue 1 year ago • 5 comments

The code originates in https://github.com/oscar-system/Oscar.jl/blob/597398c594796cd2abbd8749977cb3f2a20eefb2/experimental/GModule/GModule.jl#L1451, but I want to use it in other parts of Oscar, that should not depend on experimental. So the cleanest solution is to add it here. Note that this code currently operates in column-wise fashion (should this be changed?)

To reduce any conflicts in Oscar, it would be easiest to include this in the next breaking release, and then remove the corresponding things in https://github.com/oscar-system/Oscar.jl/blob/597398c594796cd2abbd8749977cb3f2a20eefb2/experimental/GModule/GModule.jl#L1451.

lgoettgens avatar Feb 19 '24 11:02 lgoettgens

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (b4d4de5) 86.98% compared to head (7c4c5b8) 86.96%.

:exclamation: Current head 7c4c5b8 differs from pull request most recent head de7a624. Consider uploading reports for the commit de7a624 to get more accurate results

Files Patch % Lines
src/Matrix.jl 0.00% 9 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1614      +/-   ##
==========================================
- Coverage   86.98%   86.96%   -0.03%     
==========================================
  Files         114      114              
  Lines       29686    29695       +9     
==========================================
  Hits        25823    25823              
- Misses       3863     3872       +9     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 19 '24 11:02 codecov[bot]

Not clear to me if we want this.

If we don't want this, this shouldn't be in Oscar as well (pinging @fieker).

But I think we should definitely have a cheap way to convert a matrix to a vector (or 1xn / nx1 matrix)

lgoettgens avatar Feb 19 '24 12:02 lgoettgens

On Mon, Feb 19, 2024 at 04:32:07AM -0800, Lars Göttgens wrote:

Not clear to me if we want this.

If we don't want this, this shouldn't be in Oscar as well (pinging @fieker). It is/ was neccessary, if we want this, it also needs to be done in Nemo for the Nemo types.

But I think we should definitely have a cheap way to convert a matrix to a vector

-- Reply to this email directly or view it on GitHub: https://github.com/Nemocas/AbstractAlgebra.jl/pull/1614#issuecomment-1952354492 You are receiving this because you were mentioned.

Message ID: @.***>

fieker avatar Feb 19 '24 12:02 fieker

  1. We should not have it in this form for the same reason we don't have linear indexing: It is ambiguous.
  2. Is violates A == matrix(A, nrows(A), ncols(A), vec(A))
  3. If it exists, it must be a view/reshape.

thofma avatar Feb 19 '24 13:02 thofma

I am happy to rename it to _vec in Oscar and postpone this discussion.

thofma avatar Feb 19 '24 13:02 thofma

This seems to be controversial and I also don't see a good way to implement this efficiently without a lot of effort. So is this really worth pursuing?

If @lgoettgens still has need, perhaps we should discuss it during triage?

fingolfin avatar Oct 07 '24 20:10 fingolfin

Iirc we resolved the problem in a different way

lgoettgens avatar Oct 07 '24 21:10 lgoettgens