forbid implications from non-representations to representations

[ThomasBreuer]

trafficstars

(This pull request addresses a comment in issue #2818.)

Filters of type representation are intended to describe the internal data of an object. Thus a logical implication to such a filter makes sense only if the implying filters are also representations.

As had be noticed by @fingolfin, there was a now forbidden hidden implication in the library, from IsHandledByNiceBasis to IsAttributeStoringRep. Logically, one wants this rather the other way round, that is, the mechanism relies on stored data and thus should be restricted to situations where IsAttributeStoringRep is present. I have now changed this setup accordingly, that is, IsAttributeStoringRep is no longer implied by IsHandledByNiceBasis. As a consequence, one Objectify call in the library had to be changed such that IsAttributeStoringRep gets explicitly set.

(The same might happen also in packages.)

[fingolfin]

Very nice, thank you!

This fails in testpackages -- not a surprise, we already know of several packages affected by this:

• https://github.com/gap-packages/sonata/issues/12 actually, the implication problem there does not involve representations, so it does not concern this PR here
• [x] https://github.com/gap-packages/Semigroups/issues/556 -- fix pending
• [x] https://github.com/gap-packages/numericalsgps/issues/25 -- merged and fix released in v1.1.11 (2019-03-28)
• [x] https://github.com/gap-packages/guava/pull/43
• [x] https://github.com/gap-packages/hecke/pull/6 -- merged and fix released in v1.5.2
• [x] https://github.com/semigroups/Semigroups/issues/862

As such, we'll have to wait till all packages are updated before merging this.

[fingolfin]

Four years and we still can't merge this :-(. This time the CAP package seems to do something bad (or at least it loads something which does something bad). Can't really tell what exactly from the CI output alone.

I've re-run the test manually to see where exactly it fails... got this backtrace:

InstallTrueMethod( immediate_record!.Range and cell_filter, immediate_record!.Source
  and cell_filter
 ); at /Users/mhorn/Projekte/GAP/gap.spielwiese/pkg/cap/gap/LogicForCAP.gi:404 called from
INSTALL_PREDICATE_IMPLICATION( category, immediate_record
 ); at /Users/mhorn/Projekte/GAP/gap.spielwiese/pkg/cap/gap/LogicForCAP.gi:372 called from
ADD_PREDICATE_IMPLICATIONS_TO_CATEGORY( category, current_theorem
 ); at /Users/mhorn/Projekte/GAP/gap.spielwiese/pkg/cap/gap/LogicForCAP.gi:496 called from
INSTALL_LOGICAL_IMPLICATIONS_HELPER( category, property_name
 ); at /Users/mhorn/Projekte/GAP/gap.spielwiese/pkg/cap/gap/Finalize.gi:448 called from
Finalize( cat
 ); at /Users/mhorn/Projekte/GAP/gap.spielwiese/pkg/cap/gap/CategoriesCategory.gi:718 called from
...  at /Users/mhorn/Projekte/GAP/gap.spielwiese/pkg/cap/gap/init.gi:55
...
brk> cell_filter;
<Category "CatObjectFilter">
brk> ShowImpliedFilters(cell_filter);
Implies:
   IsComponentObjectRep
   IsAttributeStoringRep
   IsCapCategoryCell
   IsCapCategoryObject
   IsCapCategoryAsCatObject

So why does CatObjectFilter is CatObjectFilter imply IsAttributeStoringRep? Well, the following declarations look fishy:

# from gap/CAP.gd:
DeclareCategory( "IsCapCategory",
                 IsAttributeStoringRep );

DeclareCategory( "IsCapCategoryCell",
                 IsAttributeStoringRep );

# from gap/Derivations.gd:
DeclareCategory( "IsDerivedMethod", IsAttributeStoringRep );
DeclareCategory( "IsDerivedMethodGraph", IsAttributeStoringRep );
DeclareCategory( "IsOperationWeightList", IsAttributeStoringRep );
DeclareCategory( "IsStringMinHeap", IsAttributeStoringRep );
....

Perhaps @zickgraf or @mohamed-barakat can say more.

[zickgraf]

I will try to comment. Disclaimer: I only have a shallow understanding of GAP's filters, categories, representations etc.

Until recently, IsCapCategory actually did not imply IsAttributeStoringRep. I changed this in https://github.com/homalg-project/CAP_project/commit/711efc931fce3c45cad8e5534860449096d37e77 for the following reasons:

1. I wanted to automatically port our code to Julia. Since Julia's type system does not allow multiple inheritance (as far as I know), I wanted to avoid joining filters with and in our code. So to avoid having to write IsAttributeStoringRep and IsCapCategory when creating a new type, I simply changed IsCapCategory to imply IsAttributeStoringRep.
2. I thought this would also make sense when looking only at GAP: CAP applies !. and SetSOME_ATTRIBUTE to CAP categories all over the place, i.e. it implicitly assumes that anything in IsCapCategory is a component object and can store attributes. So I saw no reason why IsCapCategory should not simply imply IsAttributeStoringRep, making this assumption explicit.

If this is not the intended way of doing things I'm happy for suggestions, as long as it does not involve rewriting all of CAP :D

[ThomasBreuer]

The declaration DeclareCategory( "IsCapCategory", IsAttributeStoringRep ) means that any object in the filter IsCapCategory will also lie in IsAttributeStoringRep.

I would argue that the situations where IsCapCategory objects get created with Objectify are the only places where decisions about the representations of the objects can be made. For example, there will be a contradiction if such an Objectify call creates an object in IsPositionalObjectRep, which does not allow for IsAttributeStoringRep.

If one wants IsAttributeStoringRep for certain objects then this should be said in the Objectify calls in question. I guess that there are not too many places in the code which are affected by that.

[zickgraf]

For example, there will be a contradiction if such an Objectify call creates an object in IsPositionalObjectRep, which does not allow for IsAttributeStoringRep.

Avoiding this contradiction is exactly what I was trying to achieve: If I understand https://github.com/gap-system/gap/pull/4964 correctly and if that PR would be merged, trying to create an object in IsPositionalObjectRep and IsCapCategory would give a clear error if IsCapCategory implies IsAttributeStoringRep. This is exactly what I want, because if someone creates an object in IsPositionalObjectRep and IsCapCategory and passes it to our code, it will fail in weird and unexpected ways. So I think it would be better to catch this problem right at the time of the creation of the object.

Of course we could either make our code agnostic of representations somehow or write IsCapCategory and IsAttribtueStoringRep instead of simply IsCapCategory everywhere, but that would involve touching a lot of code.

Would turning IsCapCategory into a representation solve all those problems, or could that have other unintended side-effects?

[ThomasBreuer]

@zickgraf Thanks for the explanation. In your situation, I would create a new filter with DeclareFilter( "IsCapCategoryBare" ) or so and then define IsCapCategory:= IsCapCategoryBare and IsAttributeStoringRep. Then IsCapCategory implies IsAttributeStoringRep, as wanted, and the code using this filter need not be changed. I think that technically it would also work to define IsCapCategory as a subrepresentation of IsAttributeStoringRep, but this variant looks a bit strange because it defines a new representation relative to one whose internal structure is not documented.

[zickgraf]

Thanks for your suggestion @ThomasBreuer! While trying to implement it I made the following observation:

InstallTrueMethod( IsAttributeStoringRep and NewFilter( "MyFilter2" ), IsAttributeStoringRep and NewFilter("MyFilter1") );

also throws an error. Should this be the case? If I understand this PR correctly, it says that normal filters cannot imply representations, i.e. data structures. But in this case the data structure is already part of the requirements, so I think this implication makes sense. This is the situation which triggers the error inside CAP (of course in our case IsAttributeStoringRep is hidden in some other filter, otherwise I would simply delete it from the implication).

[ThomasBreuer]

@zickgraf O.k., we can argue that an implication from A to B and C, where B describes a representation, is no problem if A is already a subset of B, and I can modify the pull request accordingly. (On the other hand, the original idea of the pull request had been to help avoiding dangerous situations in the setup of filters. In the current situation, it does not seem to be helpful.)

[zickgraf]

On the other hand, the original idea of the pull request had been to help avoiding dangerous situations in the setup of filters. In the current situation, it does not seem to be helpful.

Sorry, I don't get what you mean with the last sentence. Are you saying the PR would not be helpful anymore if the conditions for the error would be relaxed? Or that the relaxation would not be helpful because what we do in CAP is dangerous and should be fixed anyway?

[ThomasBreuer]

@zickgraf Sorry.

I am just not sure how we should proceed.

If we regard your idea as useful that attribute storing for certain objects can be forced via an implication of filters that define these objects then the current pull request makes no sense in the current form. If we still think that concluding IsAttributeStoringRep or other representations from some combination of filters (as mentioned in the initial comment) is a bad idea then some check as proposed in the pull request should be made.

At the moment it is not clear to me what exactly we want to achieve.

[zickgraf]

Some thoughts:

Maybe it makes sense to distinguish between "definitional implications" (i.e. what we currently do in CAP) and "acquired implications" (i.e. additional implications triggered via InstallTrueMethod). I think the former kind makes sense whereas the latter does not. Implementing the relaxation proposed above would respect this, i.e. allow the former but prevent the latter. And the problem which triggered this PR (#2818) falls into the later case, i.e. trigger an error, if I understand things correctly. So the PR would still achieve what it was supposed to.

[fingolfin]

@ThomasBreuer and me discussed this on Gather. I support the idea of using the logic that an implication "A => B" where B implies a rep is OK when the same rep is already part of "A". He'll work on implementing something in this vein.

[ThomasBreuer]

@zickgraf I have updated the pull request according to our discussion. Could you check whether CAP does no longer have problems with it?

[zickgraf]

@ThomasBreuer The tests of CAP, LinearAlgebraForCAP etc. now pass with the latest commit, thanks!

One question out of curiosity: This PR (both the original and the updated version) allows installing implications of formerly unrelated representations:

gap> DeclareRepresentation( "test1", IsComponentObjectRep );
gap> DeclareRepresentation( "test2", IsComponentObjectRep );
gap> InstallTrueMethod( test2, test1 );

The documentation says

The representations to which an object belongs should form a chain and either two representations are disjoint or one is contained in the other.

Thus, I cannot see a setting where InstallTrueMethod could be used for installing new implications to representations at all, except some weird cases where one wants to declare a representation without knowing its parent yet. So maybe this PR can even be strengthened to disallow any new implications to representations? I tried this by commenting the if condition right before the error

            if not ( INFO_FILTERS[j] = FNUM_REP_KERN or
                     INFO_FILTERS[j] = FNUM_REP ) then

and the only immediate issue I found was the call to InstallTrueMethod in DeclareRepresentationKernel, which can easily be avoided by using InstallTrueMethodNewFilter as in DeclareRepresentation (I'm not sure if this has other implications).

[ThomasBreuer]

@zickgraf You are right. Implications between representations should be handled in the declarations of the representations, not via InstallTrueMethod. I am going to update the pull request accordingly (although this new aspect is not covered by the title of the pull request).

[zickgraf]

I am going to update the pull request accordingly

Just checked it out and it seems to work perfectly :-)

although this new aspect is not covered by the title of the pull request

I think the title is misleading anyway after all the changes made. I suggest something like the following:

Forbid installing new implications to representations using InstallTrueMethod

[ThomasBreuer]

@zickgraf Thanks for the confirmation. I have changed the title. Now the only problem is one CI test failure concerning a problem with testexpect, see issue #5309 for details. This problem seems to be independent of the changes proposed here, thus I think that this pull request could be merged if there would be an approving review.