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

Enhance augment method in Matrix_gf2e #38453

Merged
merged 4 commits into from
Sep 29, 2024
Merged

Conversation

Shay2Shay
Copy link
Contributor

@Shay2Shay Shay2Shay commented Jul 30, 2024

This pull request modified augment() method defined in matrix_gf2e_dense to allow vectors to be augmented.
See issue: #36761

Earlier it accepted only matrix of type matrix_gf2e_dense.
I removed that dependency.
Added check for vector.
If vector then try to change the base ring of vector to self.base_ring and create matrix of type matrix_gf2e_dense from that vector
Rest of the flow remains same.

The parent class Matrix1 defined generic augment() method which allowed vectors to be augmented but matrix_gf2e_dense provides new definition of augment() and do not provide support for vectors.
For consistency it would be good to have vector augmentation in this class too.

Fixes #36761

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Jul 31, 2024

Documentation preview for this PR (built with commit 605feb4; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

You need to add a doctest showing the new behavior now works correctly.

@Shay2Shay Shay2Shay marked this pull request as draft August 6, 2024 09:43
@Shay2Shay Shay2Shay requested a review from tscrim August 27, 2024 11:57
@Shay2Shay Shay2Shay marked this pull request as ready for review August 27, 2024 12:01
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Sorry for losing track of this. LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Sep 26, 2024

Just going to wait to see if the bot comes back green with the latest develop before setting a positive review.

@vbraun vbraun merged commit c66fe84 into sagemath:develop Sep 29, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

augment() produces a TypeError for matrices over a finite field
4 participants