Add Helpers for Information about Operations and Filters #1593

markuspf · 2017-08-18T14:41:19Z

This the first part of #1236, but heavily refactored.

It adds the following

ClassOfOperation, to determine the "class" of an operation
which is one of filter, category, representation, property,
attribute, or operation.
IsCategory, IsRepresentation, IsProperty, IsAttribute
determine wheter an object is a category, property, or
attribute respectively
FilterByName retrieve filter by name
IdOfFilter return the index of a given filter in the global
list FILTERS of filters
IdOfFilterByName return the index of a filter in the global
list of filters given its name
CategoryByName return category by name

codecov · 2017-08-18T15:05:27Z

Codecov Report

Merging #1593 into master will increase coverage by <.01%.
The diff coverage is 92.39%.

@@            Coverage Diff             @@
##           master    #1593      +/-   ##
==========================================
+ Coverage   64.01%   64.01%   +<.01%     
==========================================
  Files         996      998       +2     
  Lines      327615   327604      -11     
  Branches    13213    13212       -1     
==========================================
- Hits       209717   209715       -2     
+ Misses     115043   115038       -5     
+ Partials     2855     2851       -4

Impacted Files	Coverage Δ
lib/function.g	`0% <ø> (-53.02%)`	⬇️
hpcgap/lib/function.g	`0% <ø> (-39.81%)`	⬇️
hpcgap/lib/filter.g	`89.85% <100%> (+0.3%)`	⬆️
lib/filter.g	`90% <100%> (+0.34%)`	⬆️
lib/filter.gi	`81.81% <81.81%> (ø)`
lib/function.gi	`77.58% <83.33%> (-0.2%)`	⬇️
hpcgap/lib/filter.gi	`84.61% <84.61%> (ø)`
lib/type.gi	`50.94% <96.29%> (+47.09%)`	⬆️
src/funcs.c	`72.49% <0%> (ø)`	⬆️
... and 6 more

fingolfin · 2017-08-21T13:24:11Z

lib/type.gd

+
+#############################################################################
+##
+#F  ClassOfOperation( <op> )


Looking at TAB completions for Class: there are tons of them. Perhaps call this KindOfOperation instead? Or something else?

fingolfin · 2017-08-21T13:30:16Z

lib/type.gd

+##
+##  <Description>
+##  returns <C>true</C> if <A>object</A> is a category, and <C>false</C>
+##  otherwise.


Somebody who is new to GAP and does not know about "GAP" categories, representations etc. might think that IsCategory and IsRepresentation deal with the mathematical concepts. This is unavoidable to a degree, but I think it would be helpful if we at least explain this in the help entries for them, e.g. by inserting a ref to the relevant sections on categories resp. representations.

Of course such a link wouldn't hurt for the other IsFOO functions either.

fingolfin · 2017-08-21T13:54:16Z

tst/testbugfix/2017-08-03-StringFilterSetter.tst

@@ -5,4 +5,4 @@
 gap> IS_MUTABLE_OBJ;
 <Category "IsMutable">
 gap> Setter(IS_MUTABLE_OBJ);
-<Category "<<filter-setter>>">
+<Filter "<<filter-setter>>">


Is that right? What is causing this diff?

Short story: The diff was caused by a bug that I introduced in ClassOfOperation.
Both Category and Filter are wrong though: a setter is neither a filter nor a category; I made a fix that detects setters and gives Setter as the class of the operation.

fingolfin · 2017-08-21T13:54:53Z

hpcgap/tst/testinstall/type.tst

+<Category "IsMagma">
+
+#
+gap> f := function() atomic readonly FILTER_REGION do return ForAll([1..Length(FILTERS)], id -> id = IdOfFilter(FILTERS[id])); od; end;;


Why is this wrapped into a function?

If one doesn't use the function the result (true or false) is not printed.

Aha. So how about Display(ForAll(...)) ? Or perhaps even this:

gap> atomic readonly FILTER_REGION do filters := Immutable(FILTERS); od; gap> ForAll([1..Length(filters)], id -> id = IdOfFilter(filters[id])); true

fingolfin

Except for minor nitpicks, this seems fine now.

fingolfin · 2017-08-21T16:07:37Z

lib/type.gi

+    local type, flags, types, catok, repok, propok, seenprop,
+          t, res;
+    if not IsOperation(op) then
+        ErrorNoReturn("<op> must be an operation");


Almost everywhere else, we say <oper>. I'd adjust at the very least the error message, but perhaps also rename the argument?

Point is, if I see oper anywhere in the GAP code, I immediately know what it means/is.

fingolfin · 2017-08-21T16:08:41Z

lib/type.gi

+    fi;
+
+    type := "Operation";
+    if IS_IDENTICAL_OBJ(op,IS_OBJECT) then


space after comma?

fingolfin · 2017-08-21T16:13:51Z

hpcgap/tst/testinstall/type.tst

+<Category "IsMagma">
+
+#
+gap> atomic readonly FILTER_REGION do Display(ForAll([1..Length(FILTERS)], id -> id = IdOfFilter(FILTERS[id]))); od;


So how about this:

gap> atomic readonly FILTER_REGION do filters := Immutable(FILTERS); od; gap> ForAll([1..Length(filters)], id -> id = IdOfFilter(filters[id])); true

* ClassOfOperation, to determine the "class" of an operation which is one of filter, category, representation, property, attribute, or operation. * IsCategory, IsRepresentation, IsProperty, IsAttribute determine wheter an object is a category, property, or attribute respectively * FilterByName retrieve filter by name * IdOfFilter return the index of a given filter in the global list FILTERS of filters * IdOfFilterByName return the index of a filter in the global list of filters given its name * CategoryByName return category by name

It now detects setters correctly

markuspf · 2017-08-22T12:20:59Z

Finally, tests pass so this is now ready.

fingolfin · 2017-08-22T17:01:13Z

lib/filter.g

+    x -> IS_IDENTICAL_OBJ(x, IS_OBJECT)
+         or ( IS_OPERATION( x )
+              and ( (FLAG1_FILTER( x ) <> 0 and FLAGS_FILTER(x) <> false)
+                    or x in FILTERS ) ) );


Hmm, I am not sure I understand this (both old and new version). Perhaps we can walk through it? (And perhaps the comment above it could be expanded to explain it, too?)

So first we handle IS_OBJECT/IsObject, which is special. OK. next we exclude everything that is not an operation, also fine.

But now in the old code, we look at FLAG1_FILTER( x ), and it is non-zero, we consider x a filter. Aha. But this is not quite right, as we have:

gap> FLAG1_FILTER(Setter(IsPGroup)); 625

So I guess that's why you changed this to also requite FLAGS_FILTER(x) <> false ? How about instead or additional using NARG_FUNC(x)=1 ? I simply suggest that as it is immediately clear to me why that's a necessary condition for a filter, while FLAGS_FILTER(x) <> false isn't (simply because I have to look up what it does). But anyway, if using FLAGS_FILTER is "correct", that's also fine.

So... that leaves the question: Why do we need the final or x in FILTERS? Aha:

gap> Length(FILTERS); 2212 gap> Number(FILTERS, x -> IsOperation(x) and FLAG1_FILTER(x) = 0); 905 gap> foo:=Filtered(FILTERS, x -> IsOperation(x) and FLAG1_FILTER(x) = 0);; gap> for i in [1..10] do Display(Random(foo)); od; <Filter "HasUpperCentralSeriesOfGroup"> <Filter "HasDecomposedRationalClass"> <Filter "HasOrdersClassRepresentatives"> <Filter "HasCharacteristic"> <Filter "HasFpElmEqualityMethod"> <Filter "HasBlocksAttr"> <Filter "HasLargestElementGroup"> <Filter "HasAlternatingSubgroup"> <Filter "HasExp"> <Filter "HasTestSubnormallyMonomial"> gap> HasExp; Exp; <Filter "HasExp"> <Attribute "Exp"> gap> HasAlternatingSubgroup; AlternatingSubgroup; <Filter "HasAlternatingSubgroup"> <Attribute "AlternatingSubgroup">

Aha! So testers for attributes fall through:

gap> FLAG1_FILTER(HasExp); 0 gap> FLAG2_FILTER(HasExp); 308 gap> FLAGS_FILTER(HasExp); <flag list>

Hmm...

gap> Number(FILTERS, x -> IsOperation(x) and FLAGS_FILTER(x) = false); 0 gap> FLAGS_FILTER(IS_OBJECT); <flag list> gap> FLAG1_FILTER(Exp); 0 gap> FLAG2_FILTER(Exp); 308 gap> FLAGS_FILTER(Exp); <flag list>

At this point I got a headache, and started to wonder: Why don't we add a bit "isFilter" to the header of an operation, and add an IS_FILTER function to the kernel, which just returns that bit?

Note that we don't even need to grow the operation header for this, we could split ENABLED_ATTR into a bitfield. (I'd start by adding an OperationHeader struct, as described previously)

Anyway: none of that is necessary for this PR: even adding a better comment can come in another PR. Let's just not forget...

markuspf requested a review from fingolfin August 18, 2017 14:41

markuspf force-pushed the type-information-3 branch 2 times, most recently from d9a9111 to fd37ebc Compare August 18, 2017 14:50

markuspf force-pushed the type-information-3 branch 2 times, most recently from ae303d5 to 671fac3 Compare August 18, 2017 19:38

fingolfin reviewed Aug 21, 2017

View reviewed changes

markuspf force-pushed the type-information-3 branch 2 times, most recently from 8e99fb5 to 3ad3440 Compare August 21, 2017 16:11

fingolfin approved these changes Aug 21, 2017

View reviewed changes

markuspf force-pushed the type-information-3 branch 2 times, most recently from fe8854f to 56d768e Compare August 21, 2017 18:09

markuspf force-pushed the type-information-3 branch 3 times, most recently from edb5325 to 75873f8 Compare August 22, 2017 09:01

markuspf added 4 commits August 22, 2017 10:52

Add Helpers (HPC-GAP version)

c377168

Correctly determine whether an operation is a filter

e0a7223

Update TypeOfOperation

0776127

It now detects setters correctly

Update bugfix test

e939aad

markuspf force-pushed the type-information-3 branch from 75873f8 to e939aad Compare August 22, 2017 10:05

fingolfin reviewed Aug 22, 2017

View reviewed changes

fingolfin mentioned this pull request Aug 22, 2017

Add isFilter bit to kernel operation objects, and a IS_FILTER kernel function #1615

Closed

fingolfin merged commit acf2fed into gap-system:master Aug 22, 2017

markuspf mentioned this pull request Aug 23, 2017

Additional Type Information #1236

Closed

fingolfin mentioned this pull request Sep 7, 2017

Release notes for GAP 4.9 #1699

Closed

markuspf deleted the type-information-3 branch September 8, 2017 15:11

olexandr-konovalov added this to the GAP 4.9.0 milestone Jan 20, 2018

fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label May 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Helpers for Information about Operations and Filters #1593

Add Helpers for Information about Operations and Filters #1593

markuspf commented Aug 18, 2017 •

edited

Loading

codecov bot commented Aug 18, 2017 •

edited

Loading

fingolfin Aug 21, 2017

fingolfin Aug 21, 2017

fingolfin Aug 21, 2017

fingolfin Aug 21, 2017

markuspf Aug 21, 2017

fingolfin Aug 21, 2017

markuspf Aug 21, 2017

fingolfin Aug 21, 2017

fingolfin left a comment

fingolfin Aug 21, 2017

fingolfin Aug 21, 2017

fingolfin Aug 21, 2017

markuspf commented Aug 22, 2017

fingolfin Aug 22, 2017

Add Helpers for Information about Operations and Filters #1593

Add Helpers for Information about Operations and Filters #1593

Conversation

markuspf commented Aug 18, 2017 • edited Loading

codecov bot commented Aug 18, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markuspf commented Aug 22, 2017

Choose a reason for hiding this comment

markuspf commented Aug 18, 2017 •

edited

Loading

codecov bot commented Aug 18, 2017 •

edited

Loading