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 kernel helper IS_FILTER, other tweaks #2732

Merged
merged 4 commits into from
Aug 28, 2018

Conversation

fingolfin
Copy link
Member

In particular, before this PR, we get this:

gap> Center and IsAssociative;
<Filter "(Centre and IsAssociative)">
gap> FirstOp and IsAssociative;
Error, <flags1> must be a flags list (not a boolean or fail)

Note that Center is an attribute, not a filter.

With this PR:

gap> Center and IsAssociative;
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> FirstOp and IsAssociative;
Error, <expr> must be 'true' or 'false' or a filter (not a function)

In addition, argument validation for and differed slightly between interpreter and executor, which this PR also fixes.

I am on the fence whether this should be considered relevant for the release notes. On the one hand, probably nobody ever noticed these quirks. On the other, this is a change in what kind of inputs GAP accepts...

src/opers.h Outdated
OPER(oper)->enabled = INTOBJ_INT(x);
Obj val = CONST_OPER(oper)->enabled;
Int v = val ? INT_INTOBJ(val) : 0;
OPER(oper)->enabled = INTOBJ_INT(v | 1);
Copy link
Member Author

Choose a reason for hiding this comment

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

So, this abuses enabled as a bit array. Since it now stores two bits (not just one), perhaps we should rename it. But I couldn't think of a good name -- my only idea, flags, already is used by another field in struct OperBag...

Copy link
Member

Choose a reason for hiding this comment

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

other ideas include properties (could be misunderstood), bits or bitlist (not very descriptive), options (not really options); they're all bad in some way.

With bits one could work with some macros, maybe.

@fingolfin
Copy link
Member Author

Actually I screwed up SET_ENABLED_ATTR in here. I'll fix that now and also rename enabled.

@codecov
Copy link

codecov bot commented Aug 24, 2018

Codecov Report

Merging #2732 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2732      +/-   ##
==========================================
+ Coverage   75.78%   75.78%   +<.01%     
==========================================
  Files         479      479              
  Lines      241455   241473      +18     
==========================================
+ Hits       182991   183011      +20     
+ Misses      58464    58462       -2
Impacted Files Coverage Δ
lib/filter.g 93.8% <ø> (-0.22%) ⬇️
src/opers.h 100% <100%> (ø) ⬆️
src/opers.c 94.85% <100%> (+0.01%) ⬆️
src/stats.c 94.69% <0%> (-0.2%) ⬇️
src/objset.c 84.82% <0%> (+0.22%) ⬆️
hpcgap/lib/hpc/stdtasks.g 63.93% <0%> (+0.51%) ⬆️

fingolfin added a commit to fingolfin/homalg_project that referenced this pull request Aug 24, 2018
This code was accepted by GAP and silently treated as the same
if it was using HasEvalCertainRows; but this will change in a
future GAP release.

For details see <gap-system/gap#2732>
fingolfin added a commit to fingolfin/homalg_project that referenced this pull request Aug 24, 2018
This code was accepted by GAP and silently treated as the same as in the fixed
versions; but this will change in a future GAP release.

For details see <gap-system/gap#2732>
fingolfin added a commit to fingolfin/homalg_project that referenced this pull request Aug 24, 2018
This code was accepted by GAP and silently treated as the same as in the fixed
versions; but this will change in a future GAP release.

For details see <gap-system/gap#2732>
@markuspf
Copy link
Member

Not a fan of the name extras, but with the lack of a better name, I won't stand in the way of this being merged.

@fingolfin
Copy link
Member Author

fingolfin commented Aug 24, 2018

Updated. Turns out that tests were failing due to an actual bug revealed by this PR: CanEasilyDetermineCanonicalRepresentativeExternalSet was declared as an attribute, but should have been a property. It is used for a method installation, but only in an and-filter, like so:

InstallMethod( \=, "xorbs with canonicalRepresentativeDeterminator",
  IsIdenticalObj,
  [IsExternalOrbit and CanEasilyDetermineCanonicalRepresentativeExternalSet,
   IsExternalOrbit and CanEasilyDetermineCanonicalRepresentativeExternalSet],
  0,
...

Which GAP silently accepted; the resulting filter then was IsExternalOrbit and HasCanEasilyDetermineCanonicalRepresentativeExternalSet, i.e., not at all what was intended...

I already found similar problems in some packages:

@alex-konovalov perhaps you could run package tests against this PR before we merge it? It might reveal additional similar bugs in further packages.

fingolfin added a commit to fingolfin/qpa that referenced this pull request Aug 24, 2018
IsDirectSumOfModules is not a property or attribute that can be
set to true or false due to a later computation; rather, it can only
be set when creating an object via a special constructor.

That suggests it should simply be a filter, not an attribute.

This makes qpa compatible with this upcoming GAP change:
<gap-system/gap#2732>
fingolfin added a commit to fingolfin/xmodalg that referenced this pull request Aug 24, 2018
These "attributes" only take true/false values, and have no methods,
strongly suggesting that they should be filters instead.

This fixes a conflict with a future GAP release, which will reject AND-filters
where one ore more of the "filters" involved actually is no filter (which
includes properties), but rather just an operation or attribute.

See also gap-system/gap#2732 for some technical details
@fingolfin
Copy link
Member Author

Since this breaks various packages, there are two options:

  1. We wait with merging this until all affected packages have a patch released.
  2. I update this PR to only print a (big and fat) warning) for the abuse, but otherwise accept it as before; then once all packages are fixed, we turn that back into an error.

Option 2 is a bit more work, but it has the advantage that it reduces the likelihood of new instances of this broken behavior being introduced. OTOH, I am not sure how big the likelihood for that is...

@ChrisJefferson
Copy link
Contributor

If 2 isn't too much work, it quite appeals, because it also means this doesn't get lost.

@olexandr-konovalov
Copy link
Member

This is the full list of packages where problems occur (some of they may be cause by dependencies): - homalgtocas, io_forhomalg, linearalgebraforcap, localizeringforhomalg, matricesforhomalg, modulepresentationsforcap, modules, qpa, ringsforhomalg, sco, simpcomp, toricvarieties, xmodalg. Perhaps simpcomp is not yet looked at.

@fingolfin fingolfin changed the title Add kernel helper IS_FILTER; use that to tighten rules for AND-ing filters Add kernel helper IS_FILTER, other tweaks Aug 27, 2018
@fingolfin
Copy link
Member Author

I rebased this, and for now dropped the commit which tightened the rules for AND-ing filters. Hence this PR could be merged now.

I'll put the removed commit into a new PR and/or will make a PR which just produces a warning for "bad" and-filters.

@fingolfin
Copy link
Member Author

@alex-konovalov as far as I could tell, all those packages fail due to one of homalg, qpa or xmodalg. The latter two already merged the fixes, so it's mainly waiting for @mohamed-barakat or @sebasguts to merge the homalg fix and release that.

@markuspf markuspf merged commit 068b59c into gap-system:master Aug 28, 2018
@fingolfin fingolfin deleted the mh/IS_FILTER branch August 28, 2018 10:23
@fingolfin fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Sep 21, 2018
HereAround pushed a commit to HereAround/homalg_project that referenced this pull request Jun 18, 2020
This code was accepted by GAP and silently treated as the same as in the fixed
versions; but this will change in a future GAP release.

For details see <gap-system/gap#2732>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants