Skip to content
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 Base.vec(M::MatElem) #1614

Closed
wants to merge 2 commits into from
Closed

Conversation

lgoettgens
Copy link
Collaborator

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 lgoettgens requested a review from thofma February 19, 2024 11:18
Copy link

codecov bot commented Feb 19, 2024

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%.

❗ 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 ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/Matrix.jl Show resolved Hide resolved
Copy link
Member

@thofma thofma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear to me if we want this.

@lgoettgens
Copy link
Collaborator Author

lgoettgens commented Feb 19, 2024

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)

@fieker
Copy link
Contributor

fieker commented Feb 19, 2024 via email

Co-authored-by: Max Horn <[email protected]>
@thofma
Copy link
Member

thofma commented Feb 19, 2024

  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
Copy link
Member

thofma commented Feb 19, 2024

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

@fingolfin
Copy link
Member

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?

@lgoettgens lgoettgens removed the triage label Oct 7, 2024
@lgoettgens lgoettgens closed this Oct 7, 2024
@lgoettgens lgoettgens deleted the lg/vec-Mat branch October 7, 2024 21:03
@lgoettgens
Copy link
Collaborator Author

Iirc we resolved the problem in a different way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants