Improve RecogniseGeneric and document RecogNode #308

ssiccha · 2022-01-10T13:14:52Z

Improve documentation of IsReady
Improve RecogniseGeneric
- Clean up of comments.
- Some refactoring.
- Improve the warning message if recognition of a node does not
  terminate. Since IsReady is not set, an error will be thrown once one
  wants do something with the recognition data structure. Thus it is not
  necessary to throw an error in RecogniseGeneric. Make
  sure that, if everything went fine, then IsReady is set directly
  before returning.
Rename and document recognition node constructor
- Also rename the corresponding type.
  Rename as follows:
  RecognitionInfoType -> RecogNodeType
  EmptyRecognitionInfoRecord -> RecogNode

codecov · 2022-01-10T13:28:36Z

Codecov Report

Merging #308 (cf86f68) into master (328466e) will decrease coverage by 0.01%.
The diff coverage is 75.67%.

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
- Coverage   78.61%   78.60%   -0.02%     
==========================================
  Files          43       43              
  Lines       18433    18454      +21     
==========================================
+ Hits        14492    14506      +14     
- Misses       3941     3948       +7

Impacted Files	Coverage Δ
gap/projective/almostsimple.gi	`66.04% <0.00%> (ø)`
gap/base/recognition.gi	`68.92% <76.47%> (-0.11%)`	⬇️
gap/base/recognition.gd	`100.00% <100.00%> (ø)`
gap/generic/kernel.gi	`89.51% <0.00%> (-0.57%)`	⬇️
gap/projective/c3c5.gi	`87.89% <0.00%> (+0.02%)`	⬆️

ssiccha · 2022-01-10T13:59:39Z

The lines which are not covered are mostly lines where recognition failed and I improved warning messages. I wouln't add extra tests for that since I'm a bit short on time. IMO we can ignore this.

ssiccha · 2022-01-10T14:00:39Z

@FriedrichRober Since you also looked at the last PR concerning the docs of IsReady. Could you have a look at the updated documentation of IsReady again and tell me whether you think that's again an improvement?

FriedrichRober · 2022-01-10T14:29:03Z

Yes, I will have a look today.

FriedrichRober

I only have some minor comments. The reworked documentation for IsReady is fine.

FriedrichRober · 2022-01-10T17:12:18Z

gap/base/recognition.gi

        if not IsReady(rifac) then
-            # the recognition of the image failed, also give up here:
+            # IsReady is not set, thus abort the whole computation.
            if InfoLevel(InfoRecog) = 1 and depth = 0 then Print("\n"); fi;


Should we print here some information that the computation is aborted?

Suggested change

if InfoLevel(InfoRecog) = 1 and depth = 0 then Print("\n"); fi;

if InfoLevel(InfoRecog) = 1 and depth = 0 then Print("\n"); fi;

Info(InfoRecog, 1,

"ImageRecogNode of node <ri> could not be recognised,",

" IsReady(<ri>) is not set!");

Good point. I'll add such an info message to the "unfinished case". ~~And now that I think about it, we don't need to print anything in the "image" or "kernel" parts of RecogniseGeneric, since the warning message is printed in the "unfinished case" part already.~~

Nvm, I was thinking about the mandarins. I'll update both warning messages.

FriedrichRober · 2022-01-10T17:13:25Z

gap/base/recognition.gi

        SetParentRecogNode(riker,ri);
        Info(InfoRecog,2,"Back from kernel (depth=",depth,").");

+        if not IsReady(riker) then


Here the same, maybe we should print something here?

We don't need to print something here, since we're only "exiting". The warning message was already printed in one of the "failed cases" that is in the "unfinished case" part or in the "recognise the image" part.

- Clean up of comments. - Some refactoring. - Improve the warning message if recognition of a node does not terminate. Since IsReady is not set, an error will be thrown once one wants do something with the recognition data structure. Thus it is not necessary to throw an error in RecogniseGeneric. Make sure that, if everything went fine, then IsReady is set directly before returning.

ssiccha · 2022-01-11T14:00:34Z

@FriedrichRober I think I have again made the docs of IsReady a bit more precise. Could you have another quick look? ^^

ssiccha · 2022-01-11T14:20:45Z

Ah no, wait a second. I was in Mandarin-land again.

ssiccha · 2022-01-11T14:27:04Z

Ok, now the docs of IsReady should be good to go. The version before the Fix docs of IsReady should be correct once Mandarins are implemented. But we'll see that once the Mandarins are actually finished.

FriedrichRober · 2022-01-12T09:30:32Z

The doc of IsReady sounds now a bit more technical, but I understand what is going on. I approve.

fingolfin · 2022-01-19T14:02:36Z

gap/base/recognition.gi

+  end );
+
+InstallOtherMethod( RecogNode,
+  "for backwards compatibility",


What are you trying to be backwards compatible with, though? is there still code in the repo that calls it like that? If so, can't we just update that code?

Other than that: any external code won't be calling RecogNode, as it didn't exist before, right? It'll call EmptyRecognitionInfoRecord, so that code needs to be adjusted anyway. Or perhaps we can retain EmptyRecognitionInfoRecord in a new file gap/obsolete.gi; but then that backwards compatible function can take of reordering the arguments.

Perhaps I am missing something, though?

Perhaps I am missing something, though?

Nope, that makes perfect sense, thanks! 👍

I'm going with this:

Or perhaps we can retain EmptyRecognitionInfoRecord in a new file gap/obsolete.gi; but then that backwards compatible function can take of reordering the arguments.

fingolfin

Looks good to me (but note my comment on the backwards compatibility method)

ssiccha · 2022-01-26T06:34:01Z

I had to fiddle around a bit with the backwards compatibility method, but now it works on my machine. I'll wait for tests to pass, then this gets merged.

Nope, there is an unbound global variable somewhere.

Also rename the corresponding type. Rename as follows: RecognitionInfoType -> RecogNodeType EmptyRecognitionInfoRecord -> RecogNode

ssiccha added the enhancement label Jan 10, 2022

ssiccha requested review from AnnaKDS, FriedrichRober, danielrademacher and fingolfin January 10, 2022 13:14

ssiccha force-pushed the ss/improve-RecogniseGeneric branch from 55c6f1d to d2f34bf Compare January 10, 2022 13:19

ssiccha force-pushed the ss/improve-RecogniseGeneric branch from d2f34bf to 19ef011 Compare January 10, 2022 14:32

FriedrichRober approved these changes Jan 10, 2022

View reviewed changes

Improve documentation of IsReady

f2595f0

ssiccha force-pushed the ss/improve-RecogniseGeneric branch from 19ef011 to 96ca419 Compare January 11, 2022 13:42

ssiccha force-pushed the ss/improve-RecogniseGeneric branch from 96ca419 to d6b1fcf Compare January 11, 2022 13:58

ssiccha mentioned this pull request Jan 12, 2022

Implement mandarins (WIP) #179

Draft

fingolfin reviewed Jan 19, 2022

View reviewed changes

fingolfin approved these changes Jan 19, 2022

View reviewed changes

ssiccha force-pushed the ss/improve-RecogniseGeneric branch from dc93ddd to 5c08ab7 Compare January 26, 2022 06:33

ssiccha force-pushed the ss/improve-RecogniseGeneric branch 2 times, most recently from 7f4b653 to c0d88de Compare January 26, 2022 07:09

ssiccha added 2 commits January 26, 2022 08:22

Rename and document recognition node constructor

bef7f68

Also rename the corresponding type. Rename as follows: RecognitionInfoType -> RecogNodeType EmptyRecognitionInfoRecord -> RecogNode

Fix docs of IsReady

cf86f68

ssiccha force-pushed the ss/improve-RecogniseGeneric branch from c0d88de to cf86f68 Compare January 26, 2022 07:22

ssiccha merged commit 4349d80 into gap-packages:master Jan 31, 2022

ssiccha deleted the ss/improve-RecogniseGeneric branch January 31, 2022 05:36

-            if InfoLevel(InfoRecog) = 1 and depth = 0 then Print("\n"); fi;
+            if InfoLevel(InfoRecog) = 1 and depth = 0 then Print("\n"); fi;
+            Info(InfoRecog, 1,
+                 "ImageRecogNode of node <ri> could not be recognised,",
+                 " IsReady(<ri>) is not set!");

Improve RecogniseGeneric and document RecogNode #308

Improve RecogniseGeneric and document RecogNode #308

Uh oh!

Conversation

ssiccha commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ssiccha commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ssiccha commented Jan 10, 2022

Uh oh!

FriedrichRober commented Jan 10, 2022

Uh oh!

FriedrichRober left a comment

Choose a reason for hiding this comment

Uh oh!

FriedrichRober Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

ssiccha Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FriedrichRober Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

ssiccha Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

ssiccha commented Jan 11, 2022

Uh oh!

ssiccha commented Jan 11, 2022

Uh oh!

ssiccha commented Jan 11, 2022

Uh oh!

FriedrichRober commented Jan 12, 2022

Uh oh!

fingolfin Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

ssiccha Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

ssiccha Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

fingolfin left a comment

Choose a reason for hiding this comment

Uh oh!

ssiccha commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ssiccha commented Jan 10, 2022 •

edited

Loading

codecov bot commented Jan 10, 2022 •

edited

Loading

ssiccha commented Jan 10, 2022 •

edited

Loading

ssiccha Jan 11, 2022 •

edited

Loading

ssiccha commented Jan 26, 2022 •

edited

Loading