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

Remove four argument mul! for ZZMatrix #1857

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

fingolfin
Copy link
Member

Remove four argument mul! for ZZMatrix. The fourth argument was a Bool, which if set to true would basically perform an addmul! (except that we don't have addmul! for matrices right now...).

This code was introduced by @fieker so pinging him in case he remembers what it was for.

I also made Nemocas/AbstractAlgebra.jl#1801 which once merged and released should produce identical results for ZZMatrix inputs.

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.25%. Comparing base (4d25cd4) to head (9eafea2).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1857      +/-   ##
==========================================
+ Coverage   86.21%   86.25%   +0.03%     
==========================================
  Files          99       99              
  Lines       36030    36027       -3     
==========================================
+ Hits        31064    31075      +11     
+ Misses       4966     4952      -14     

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

@lgoettgens
Copy link
Collaborator

We should merge for downstream tests working again before merging this

@fieker
Copy link
Contributor

fieker commented Sep 19, 2024 via email

@lgoettgens
Copy link
Collaborator

We (@thofma and I) may have already broken the generic version of this in AA. I'll look into it tomorrow

@lgoettgens lgoettgens self-assigned this Sep 19, 2024
@thofma
Copy link
Member

thofma commented Sep 19, 2024

@lgoettgens
Copy link
Collaborator

@fieker I have not found it in https://github.com/Nemocas/AbstractAlgebra.jl/blob/0f401540af7cf9d0b7880e13e8766700c0e65d0d/src/Matrix-Strassen.jl#L3

As I wrote above, I have probably messed this up...

@lgoettgens
Copy link
Collaborator

Strassen is broken in AA v0.43.1. Nemocas/AbstractAlgebra.jl#1803 will revive it again, but then it does not need this four-argument mul! anymore.
So in any case, we can remove it.

@lgoettgens lgoettgens closed this Sep 20, 2024
@lgoettgens lgoettgens reopened this Sep 20, 2024
@lgoettgens lgoettgens enabled auto-merge (squash) September 20, 2024 17:49
@fingolfin
Copy link
Member Author

@lgoettgens you enabled and then disabled auto-squash. Just wondering if there are any remaining concerns, or was this just an accident?

@lgoettgens lgoettgens merged commit 8ce122a into Nemocas:master Sep 21, 2024
48 checks passed
@lgoettgens
Copy link
Collaborator

Just instable internet on the train

@fingolfin fingolfin deleted the mh/addmul branch September 21, 2024 08:51
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