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 new kernel operations ELM_MAT, ASS_MAT #3551

Merged
merged 3 commits into from
Jul 12, 2019

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Jul 11, 2019

This continues the work begun in PR #3550.

The kernel functions with the same name now delegate to these operations instead of ELM_LIST, ASS_LIST.

Also turned MatElm, SetMatElm into synonyms for ELM_MAT, ASS_MAT.

Code which already used MatrixObj for a long time and thus installed methods for MatElm, SetMatElm will thus work unchanged. Code which adapted to the recommendation in GAP 4.10 and thus instead installed methods for ELM_LIST, ASS_LIST with 3 resp. 4 arguments now should be adapted to (again) install methods for MatElm, SetMatElm for maximal compatibility. To ensure this code
works after this change, though, we provide low ranked fallback methods in ELM_MAT, ASS_MAT which delegate to ELM_LIST, ASS_LIST. At the same time, we remove the low-ranked delegations from ELM_LIST, ASS_LIST to MatElm, SetMatElm to ensure we don't end up delegating back and forth endlessly.

As far as I could tell, the only package which is (mildly) impacted by this is cvec; I'll shortly release an update of that which addresses all those concerns, though, and which should work seamlessly with and without this PR. Of course there might be further code out there which is not in deposited packages, and which I thus can't test, but I am confident it'll be OK, too -- though of course I can't be sure without being able to look at that code.

@fingolfin fingolfin added topic: kernel release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jul 11, 2019
@fingolfin fingolfin added this to In progress in MatrixObj via automation Jul 11, 2019
InstallMethod( \[\], "for GF2 matrix",
[ IsGF2MatrixRep,
IsPosInt, IsPosInt ],
MAT_ELM_GF2MAT );
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now redundant as we already do InstallMethod(MatElm, ..., MAT_ELM_GF2MAT); later in the file.

[ IsGF2MatrixRep,
IsPosInt, IsPosInt,
IsObject ],
SET_MAT_ELM_GF2MAT );
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now redundant as we already do InstallMethod(SetMatElm, ..., SET_MAT_ELM_GF2MAT); later in the file.

lib/mat8bit.gi Outdated
@@ -88,7 +88,7 @@ InstallOtherMethod( \[\], "For a compressed MatFFE",
#M <mat> [ <pos1>, <pos2> ]
##

InstallMethod( \[\], "For a compressed MatFFE",
InstallMethod( \[\,\], "For a compressed MatFFE",
Copy link
Member Author

Choose a reason for hiding this comment

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

We could of course also use MatElm here or ELM_MAT... shrug

[ IsPlistMatrixRep and IsMutable, IsPosInt, IsPosInt, IsObject ],
function( m, row, col, ob )
m![ROWSPOS][row]![ELSPOS][col] := ob;
end );
Copy link
Member Author

Choose a reason for hiding this comment

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

Redundant, as we already (resp.: still) install identical methods for MatElm and SetMatElm above

@fingolfin
Copy link
Member Author

fingolfin commented Jul 11, 2019

One question to ask here is: What about ISB_MAT and UNB_MAT? Should they also become kernel operations, with aliases IsBound\[\,\] and Unbind\[\,\]?

Thing is, I am not convinced these operations are useful at all. I wouldn't include them in the MatrixObj specification at all. I am in fact tempted to deprecate the syntax IsBound(mat[i,j]) and Unbind(mat[i,j]), or even outright abolish it. I just can't think of any good use for matrices; and for more general tables, well, you still can do Unbind(table[i][j]) ... Thoughts, comments?

Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

This looks good.
I have tried MethodsOperation( \[\], 3 ) etc., and found that also the packages in GAPJulia are affected.
(And the MatFE vs MatFFE typos could be corrected.)

lib/mat8bit.gi Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member Author

@ThomasBreuer re GAPJulia: ah good -- I didn't check that. I did, however, check MethodsOperation after doing a LoadAllPackage with the standard set of distributed packages. But of course that misses any packages which are not distributed, as well as not yet released versions.

fingolfin added a commit to fingolfin/cvec that referenced this pull request Jul 11, 2019
fingolfin added a commit to gap-packages/cvec that referenced this pull request Jul 11, 2019
The kernel functions with the same name now delegate to these operations
instead of ELM_LIST, ASS_LIST.

Also turned MatElm, SetMatElm into synonyms for ELM_MAT, ASS_MAT.

Code which uses MatrixObj for a long time and thus installed methods for
MatElm, SetMatElm will thus work unchanged. Code which adapted to the
recommendation in GAP 4.10 and thus instead installed methods for ELM_LIST,
ASS_LIST with 3 resp. 4 arguments now should be adapted to (again) install
methods for MatElm, SetMatElm for maximal compatibility. To ensure this code
works after this change, though, we provide low ranked fallback methods in
ELM_MAT, ASS_MAT which delegate to ELM_LIST, ASS_LIST. At the same time, we
remove the low-ranged delegations from ELM_LIST, ASS_LIST to MatElm,
SetMatElm to ensure we don't end up delegating back and forth endlessly.
... and not "For" -- this matches the style in the rest of the library.
@codecov
Copy link

codecov bot commented Jul 11, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@0e29ceb). Click here to learn what that means.
The diff coverage is 94.4%.

@@            Coverage Diff            @@
##             master    #3551   +/-   ##
=========================================
  Coverage          ?   84.19%           
=========================================
  Files             ?      695           
  Lines             ?   345053           
  Branches          ?        0           
=========================================
  Hits              ?   290512           
  Misses            ?    54541           
  Partials          ?        0
Impacted Files Coverage Δ
lib/matobjplist.gi 48.38% <ø> (ø)
lib/vec8bit.gi 79.52% <100%> (ø)
lib/matblock.gi 91.87% <100%> (ø)
lib/ffeconway.gi 86.5% <100%> (ø)
hpcgap/lib/vecmat.gi 82.92% <100%> (ø)
lib/mat8bit.gi 79.6% <100%> (ø)
lib/vecmat.gi 85.52% <100%> (ø)
hpcgap/lib/vec8bit.gi 79.38% <100%> (ø)
lib/grpfp.gi 78.29% <100%> (ø)
lib/matobj2.gd 100% <100%> (ø)
... and 2 more

@coveralls
Copy link

coveralls commented Jul 11, 2019

Coverage Status

Coverage decreased (-0.001%) to 84.299% when pulling ea1a88c on fingolfin:mh/ELM_MAT-oper into 0e29ceb on gap-system:master.

@fingolfin fingolfin merged commit b394844 into gap-system:master Jul 12, 2019
MatrixObj automation moved this from In progress to Done Jul 12, 2019
@fingolfin fingolfin deleted the mh/ELM_MAT-oper branch July 12, 2019 12:43
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 20, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
MatrixObj
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants