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 indentation when printing double cosets #5841

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

ThomasBreuer
Copy link
Contributor

The distribution of \< and \> hints was obviously wrong.

I still do not understand why the hints were chosen this way. Usually the idea is to mark positions in the string where line breaks are more suitable than in other places, for example at the commas that separate objects; but here it is the other way round?

resolves #5840

The distribution of `\<` and `\>` hints was obviously wrong.

I still do not understand why the hints were chosen this way.
Usually the idea is to mark positions in the string where line breaks
are more suitable than in other places, for example at the commas
that separate objects; but here it is the other way round?
@fingolfin fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Nov 13, 2024
@@ -536,7 +536,7 @@ function(d)
return(STRINGIFY("DoubleCoset(\<",
Copy link
Member

Choose a reason for hiding this comment

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

This is indeed weird. I would expect it to start with \> not \<. More like so

Suggested change
return(STRINGIFY("DoubleCoset(\<",
return(STRINGIFY("DoubleCoset(\>",
ViewString(LeftActingGroup(d)),"\<,\>",
ViewString(Representative(d)),"\<,\>",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point is that I do not know what the idea behind this code was.
Also the fact that whitespace behind the commas is missing is unusual.
I guess this code was never really used, otherwise the indentation problem would have been noticed earlier.
Shall we just change this method?

Copy link
Member

Choose a reason for hiding this comment

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

Shall we just change this method?

Fine by me.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

This obviously is better than what was before, but still looks strange (similar for the ViewString method for (right) cosets)

@ThomasBreuer
Copy link
Contributor Author

Another irritating behaviour:
When debugging the problem, I tried the following:

gap> G:= SymmetricGroup(5);;
gap> H:= SymmetricGroup(3);;
gap> K:= SymmetricGroup(2);;
gap> dc:= DoubleCosets(G, H, K);;
gap> meth:= ApplicableMethod( ViewObj, [ dc ], 1, "all" );;
gap> meth[1]( dc );  # a method from "teaching mode" ...
"TRY_NEXT_METHOD"
gap> meth[2]( dc );
Error, SetPrintObjIndex: bad state, PrintObjDepth is 0 in
  SET_PRINT_OBJ_INDEX( i ); at /usr/local/lib/gap-rsync/lib/list.gi:3886 called from 
<function "ViewObj for a finite list">( <arguments> )
 called from read-eval loop at *stdin*:7
[ 

Calling the ViewObj method explicitly runs into an error, whereas the same method works when it gets called from View?

@fingolfin
Copy link
Member

The kernel operations ViewObj and PrintObj involve some special magic in order to handle printing of recursive structures, i.e. of lists and records which directly or indirectly contain themselves. So that e.g. l:=[];l[1]:=l; gets printed as [ ~ ].

The printing methods implemented in GAP code call the kernel function SET_PRINT_OBJ_INDEX for this purpose, but that is only allowed "during" a ViewObj resp. PrintObj invocation.

That's why the method does not work when called in isolation.

@fingolfin
Copy link
Member

This fixes an error so I'll merge it. Perhaps the printing can be improved further but that can be done in a follow-up PR

@fingolfin fingolfin merged commit 329ab54 into gap-system:master Nov 14, 2024
33 checks passed
@ThomasBreuer
Copy link
Contributor Author

O.k.

I had a look at the \< and \> handling in general. There are not too many places where these hints occur, but they are not consistent.

(Also, documentation is missing, and comments in the code are talking about "indentation", which I find confusing.)

@ThomasBreuer ThomasBreuer deleted the TB_double_cosets_view branch November 14, 2024 17:10
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

strange indentation in output
2 participants