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

fix access to matrix entries for the GAP master branch #279

Merged

Conversation

ThomasBreuer
Copy link
Member

GAP 4.10 calls the operation \[\] with 3 arguments,
the master branch calls the new operation \[\,\].

Note that the default delegation from \[\,\] to \[\]
does not apply in the given situation because the GAP objects
are just Julia objects, without type information about IsMatrixObj.

(After this change, the tests that were broken after merging #276 should pass again.)

GAP 4.10 calls the operation `\[\]` with 3 arguments,
the master branch calls the new operation `\[\,\]`.

Note that the default delegation from `\[\,\]` to `\[\]`
does not apply in the given situation because the GAP objects
are just Julia objects, without type information about `IsMatrixObj`.
@ThomasBreuer ThomasBreuer requested a review from fingolfin July 15, 2019 15:40
@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

Merging #279 into master will decrease coverage by 0.25%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
- Coverage   69.05%   68.79%   -0.26%     
==========================================
  Files          54       54              
  Lines        3564     3580      +16     
==========================================
+ Hits         2461     2463       +2     
- Misses       1103     1117      +14
Impacted Files Coverage Δ
pkg/GAPJulia/JuliaInterface/gap/adapter.gi 82.66% <81.25%> (-15.64%) ⬇️
pkg/GAPJulia/JuliaExperimental/gap/numfield.g 95.45% <0%> (-0.33%) ⬇️

#TODO: Remove the installations for '\[\]' and '\[\]\:\='
# as soon as a released GAP version supports '\[\,\]' and '\[\,\]\:\='.
if IsBound( \[\,\] ) then
InstallOtherMethod( \[\,\],
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just install methods for MatElm and SetMatElm? At least that was the theory, that this way one could write the code so that it works with 4.9, 4.10 and 4.11

Copy link
Member

Choose a reason for hiding this comment

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

Argh, no of course not, and you already explained the reason in your PR description. Sorry about that

@ThomasBreuer ThomasBreuer merged commit e5a6434 into oscar-system:master Jul 18, 2019
@ThomasBreuer ThomasBreuer deleted the TB_access_matrix_elements branch July 18, 2019 11:07
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.

2 participants