Ranges: improve printing and conversion to strings #4625

fingolfin · 2021-08-02T22:39:47Z

Add tests for printing ranges & converting them to strings
Improve String/ViewString/PrintString for ranges

The diff between the first and second commit shows what changed.

Perhaps we should have similar tests for other library objects?

This patch has been motivated by the CI failure I observed here: https://github.com/gap-packages/fr/pull/38/checks?check_run_id=3160342361

fingolfin · 2021-08-02T22:57:29Z

tst/testinstall/range.tst

+PrintString: [ 0 .. 2 ]
+String: [ 0 .. 2 ]
+gap> TestPrintRangeRep([0,2..4]);
+ViewObj: [ 0, 2 .. 4 ]


I guess we could go one step further and also print [ 0, 2 .. 4 ] as [ 0, 2, 4 ]...

The input [ 0, 2, 4 ] does not produce an object in IsRangeRep, whereas [ 0, 2 .. 4 ] creates such an object.
In this sense, I would not be in favour of this change.

Note that the documentation does not make any claim that the representation is preserved by reentering the output of Print; just that an equal object is returned. So both print outputs conform to the documentation.

In any case, always printing ranges as ranges certainly is also an option; the main motivation for this PR really is to make it behave consistently in one way or the other.

I would support this kind of change, if you want to make it.

fingolfin · 2021-08-04T13:34:40Z

Some tests here failing, which will be easy enough to fix, but it'd be nice to first get feedback on the general idea.

fingolfin · 2021-08-04T13:37:15Z

tst/testinstall/range.tst

+DisplayString: <object>
+ViewString: [ 0, -1 ]
+PrintString: [ 0, -1 ]
+String: [ 0, -1 ]


Note: perhaps we do want this to produce [ 0, -1 .. -1 ] in a few cases; and likewise, [ 0 .. 1 ] instead of [ 0, 1 ]. But then IMHO these two should behave consistenly: either both kinds of ranges of length 2 are shown "as range", or both are shown "as lists". Also, of course ViewString and ViewObj should be consistent, and likewise PrintString and PrintObj. (Well... and DisplayString, but I did not even try to fix that in this PR)

wilfwilson · 2021-08-04T13:50:23Z

I don't have time to properly review at the moment, but I do like the general idea from what I can get at a glance. However, I worry that it suffers from the same problem as #4050, i.e. breaks the tests of various packages, which will make it annoying to merge.

ThomasBreuer

Let me try to summarize what this pull request changes.

For ranges of length two, Display, PrintObj, PrintString, and String now show the two entries separated by a comma; up to now, the .. notation was used. The output of ViewObj (using a comma) was not changed. ViewString uses a comma before and after the change, but the hints \> and \< that were present before the change have now disappeared.
For ranges with at least three elements, now there is a special ViewString method that uses the .. notation; up to now, all entries of the range were listed. I regard the pull request as a bugfix in this respect. (One could discuss whether the hints \> and \< should be inserted.)

From the viewpoint of consistency w.r.t. the operations in question, now ranges with at least three elements are shown as ranges if they store that they are ranges, and ranges of length two are not shown as ranges. (Ranges of length at most one do not store that they are ranges.)

Concerning the impact of the change on package tests, I have not found differences in the basic tests for my packages CTblLib, AtlasRep, and CTBlocks. Perhaps there are some differences in the tests of other packages, but I think that the changes proposed in #4050 (mentioned by @wilfwilson ) cause significantly more trouble.

wilfwilson

I've looked at this in more detail now and I do support it the changes, in particular the consistency and the showing length-two ranges like [1..2] as [1,2]. I would support further such changes in this direction (e.g. [1,3..5] being [1,3,5]).

@ThomasBreuer raises a good point about the \> and \< hints.

I agree with @ThomasBreuer that this PR should not be anyway near as disruptive as #4050, but it causes at least some problems. Running the standard tests of Digraphs and Semigroups with this PR I get:

Digraphs: 6 failures in 2 of 16 files
Semigroups: 11 failures in 3 of 67 files

It should be easy to fix such problems by suppressing output. Hopefully it will be easy enough to detect affected packages.

Note that the CI is currently not passing.

wilfwilson · 2021-08-20T15:23:05Z

tst/testinstall/range.tst

+PrintString: [ 0 .. 2 ]
+String: [ 0 .. 2 ]
+gap> TestPrintRangeRep([0,2..4]);
+ViewObj: [ 0, 2 .. 4 ]


I would support this kind of change, if you want to make it.

wilfwilson · 2021-08-20T15:25:19Z

tst/testinstall/reesmat.tst

@@ -270,7 +270,7 @@ false

 # IsReesMatrixSemigroup: for a Rees matrix subsemigroup with generators, 1
 gap> R := ReesMatrixSemigroup(SymmetricGroup(2), [[(1,2)]]);
-<Rees matrix semigroup 1x1 over Sym( [ 1 .. 2 ] )>
+<Rees matrix semigroup 1x1 over Sym( [ 1, 2 ] )>


There are quite a few of these kinds of changes in Semigroups and Digraphs, (almost?) all of which involve Sym( [ 1 .. 2 ] ) becoming Sym( [ 1, 2 ] )

olexandr-konovalov

Would be nice to have - will impact package tests, but with the advent of https://github.com/gap-system/PackageDistro/ that can be then systematically addressed. Some packages may need more work to adapt examples to pass in both new and old GAP versions.

... to match ViewObj and Display. As a result, Display, PrintObj, DisplayString, PrintString and String now all behave the same for ranges, while ViewObj and ViewString differ. This is probably quite surprising to people, but changing it one way or the other will affect packages, so for now let's play it safe and only address the "obvious" bugs.

fingolfin · 2022-08-03T08:51:50Z

I have now revised the changes in this PR, going in a somewhat different direction: instead of trying to align ViewObj with PrintObj and Display, I've opted to only "fix" ViewString for ranges to produce a string corresponding to the output of ViewObj. Also, I've now added a DisplayString method matching Display.

The result is still a bit odd because now the View* methods (still) behave differently from the Display* and Print* methods. But at least one inconsistency was resolved, and I don't think there will be any conflicts with packages. If someone wants to resolve the remaining inconsistency one day, be my guess, but I don't think it's all that important.

pedritomelenas

I wonder if lists with three elements like [0,2..4] should be “printed” as [0, 2, 4] instead of [0, 2 .. 4]

fingolfin · 2022-08-04T18:01:24Z

"Should"? Dunno.... "Could"? Sure. But any such change has a cost, you need to at least update all packages whose test suites depend on the printing. IMHO it is not worth the trouble

pedritomelenas · 2022-08-05T00:23:15Z

Sure, I should have used could.

fingolfin added topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Aug 2, 2021

fingolfin requested review from olexandr-konovalov and wilfwilson August 2, 2021 22:39

fingolfin force-pushed the mh/unify-range-printing-viewing branch from 52ba080 to dfb7a43 Compare August 2, 2021 22:55

fingolfin commented Aug 2, 2021

View reviewed changes

fingolfin requested a review from ThomasBreuer August 4, 2021 13:33

fingolfin commented Aug 4, 2021

View reviewed changes

fingolfin force-pushed the mh/unify-range-printing-viewing branch from dfb7a43 to a3c09ff Compare August 4, 2021 13:39

ThomasBreuer approved these changes Aug 4, 2021

View reviewed changes

wilfwilson approved these changes Aug 20, 2021

View reviewed changes

fingolfin force-pushed the mh/unify-range-printing-viewing branch from a3c09ff to d554eca Compare January 9, 2022 14:10

olexandr-konovalov reviewed May 2, 2022

View reviewed changes

fingolfin force-pushed the mh/unify-range-printing-viewing branch from d554eca to 9a2e32e Compare August 2, 2022 12:22

fingolfin added 2 commits August 3, 2022 10:32

Add tests for printing ranges & converting them to strings

d8fc20f

fingolfin force-pushed the mh/unify-range-printing-viewing branch from 9a2e32e to de03049 Compare August 3, 2022 08:49

fingolfin requested review from ThomasBreuer and wilfwilson August 3, 2022 08:51

wilfwilson approved these changes Aug 3, 2022

View reviewed changes

fingolfin merged commit b4e599e into gap-system:master Aug 3, 2022

fingolfin deleted the mh/unify-range-printing-viewing branch August 3, 2022 20:29

pedritomelenas reviewed Aug 4, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ranges: improve printing and conversion to strings #4625

Ranges: improve printing and conversion to strings #4625

fingolfin commented Aug 2, 2021 •

edited

Loading

fingolfin Aug 2, 2021

ThomasBreuer Aug 4, 2021

fingolfin Aug 4, 2021

wilfwilson Aug 20, 2021

fingolfin commented Aug 4, 2021

fingolfin Aug 4, 2021 •

edited by wilfwilson

Loading

wilfwilson commented Aug 4, 2021

ThomasBreuer left a comment

wilfwilson left a comment

wilfwilson Aug 20, 2021

wilfwilson Aug 20, 2021

olexandr-konovalov left a comment

fingolfin commented Aug 3, 2022 •

edited by wilfwilson

Loading

pedritomelenas left a comment

fingolfin commented Aug 4, 2022

pedritomelenas commented Aug 5, 2022

Ranges: improve printing and conversion to strings #4625

Ranges: improve printing and conversion to strings #4625

Conversation

fingolfin commented Aug 2, 2021 • edited Loading

fingolfin Aug 2, 2021

Choose a reason for hiding this comment

ThomasBreuer Aug 4, 2021

Choose a reason for hiding this comment

fingolfin Aug 4, 2021

Choose a reason for hiding this comment

wilfwilson Aug 20, 2021

Choose a reason for hiding this comment

fingolfin commented Aug 4, 2021

fingolfin Aug 4, 2021 • edited by wilfwilson Loading

Choose a reason for hiding this comment

wilfwilson commented Aug 4, 2021

ThomasBreuer left a comment

Choose a reason for hiding this comment

wilfwilson left a comment

Choose a reason for hiding this comment

wilfwilson Aug 20, 2021

Choose a reason for hiding this comment

wilfwilson Aug 20, 2021

Choose a reason for hiding this comment

olexandr-konovalov left a comment

Choose a reason for hiding this comment

fingolfin commented Aug 3, 2022 • edited by wilfwilson Loading

pedritomelenas left a comment

Choose a reason for hiding this comment

fingolfin commented Aug 4, 2022

pedritomelenas commented Aug 5, 2022

fingolfin commented Aug 2, 2021 •

edited

Loading

fingolfin Aug 4, 2021 •

edited by wilfwilson

Loading

fingolfin commented Aug 3, 2022 •

edited by wilfwilson

Loading