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

in general, do not use a known Name value in PrintObj #5701

Closed

Conversation

ThomasBreuer
Copy link
Contributor

@ThomasBreuer ThomasBreuer commented Apr 16, 2024

This addresses #5699.

For more than 25 years, there is a PrintObj method with high rank that just prints a known Name value of the object in question.
According to the documentation of Name, a known Name value should be used only by View and ViewObj; the function Print is recommended to show GAP input to reconstruct the object in question if this makes sense.

When we remove the PrintObj method in question, the tests from testinstall and teststandard show differences essentially in two situations:

  • Certain global variables such as Rationals are currently expected to be printed via this name. The solution proposed here is to provide a PrintObj method with high rank that uses a known name under the condition that a global variable with this name exists individual PrintObj methods for the objects in question. This works because there are already filters that describe these objects.
  • Some tests for groups returned by SimpleGroupsIterator call Print in order to show the names of these groups. Here we can call View instead of Print.

(Let us see the results of the other tests, perhaps there are more differences.)

@ThomasBreuer ThomasBreuer added topic: library kind: quirk Issues that are not bugs, but a discrepancy between user expectation and system behavior release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Apr 16, 2024
@ThomasBreuer
Copy link
Contributor Author

No, apparently the CI tests do not complain about the proposed change.
So is this a reasonable solution to Leonard's problem?

@Stefan-Kohl
Copy link
Member

Would this really be a good idea? -- I mean, for example assume you name an object "X". Then, as I understand, with your proposal this would still be "Print"ed as "X", as there is a global variable named "X". (Or did I misread something?)

@ThomasBreuer
Copy link
Contributor Author

@Stefan-Kohl

Would this really be a good idea? -- I mean, for example assume you name an object "X". Then, as I understand, with your proposal this would still be "Print"ed as "X", as there is a global variable named "X". (Or did I misread something?)

Well, this has been the behaviour since more than 25 years, and nobody has reported a problem with it.
Yes, one can set misleading names. If a user does this then it is his/her fault, and View will continue showing these names. (I think that function names such as X cause less confusion than names of objects such as Rationals.)

I think that Name should not be set by library functions, but this is another question to be answered.

Copy link
Contributor

@james-d-mitchell james-d-mitchell left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Member

@frankluebeck frankluebeck left a comment

Choose a reason for hiding this comment

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

I do not support this new method. The way, an object is printed should not depend on the state of the GAP session in this way. (A global name can become bound and this may have no relation with the object to be printed.)

I would vote for removing this method completely.

Or, just leave it as it always was and adjust the documentation (if the additional work caused by removing it seems too much).

Also, I don't think that the proposed change addresses the issue #5699 appropriately. That should be done with better methods for viewing and printing those objects.

lib/object.gi Outdated
Comment on lines 388 to 389
if ISBOUND_GLOBAL( Name( obj ) ) then
Print( Name( obj ) );
Copy link
Member

Choose a reason for hiding this comment

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

I think this should only be triggered if the variable with that name actually contains the object. I.e. like so:

Suggested change
if ISBOUND_GLOBAL( Name( obj ) ) then
Print( Name( obj ) );
local name;
name := Name( obj );
if ISBOUND_GLOBAL( name ) and IS_IDENTICAL_OBJ( VALUE_GLOBAL( name ), obj ) then
Print( name );

lib/object.gi Outdated
@@ -384,7 +384,13 @@ InstallMethod( PrintObj,
true,
[ HasName ],
SUM_FLAGS, # override anything specific
function ( obj ) Print( Name( obj ) ); end );
function( obj )
Copy link
Member

Choose a reason for hiding this comment

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

@ThomasBreuer perhaps less controversial: just remove this method, and add back dedicated printing methods for Rationals and other objects relying on this. This assumes there are only a few (Rationals, Integers, GaussianRationals, ... come to mind).

If there are sp many as to make this annoying, we could also introduce a new attribute NameForPrinting and use that in here.


That said, I wonder how badly this PR breaks package CI tests. Perhaps you can update and rebase this PR (resp. merge current master into it), then I could trigger a run of the PackageDistro CI tests against this pull request to see where it stands.

Copy link
Contributor Author

@ThomasBreuer ThomasBreuer Oct 15, 2024

Choose a reason for hiding this comment

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

O.k., I have updated the pull request (and also its description) accordingly.

Use a known `Name` value in `PrintObj` only if a global variable
with this name exists.
- remove the offending `PrintObj` method
- install individual `PrintObj` methods for the few objects
  that are affected (in our test suite) by this change
@fingolfin
Copy link
Member

fingolfin commented Oct 18, 2024

Causes failures in the test suites of 18 9 GAP packages, see https://github.com/gap-system/PackageDistro/actions/runs/11401453048

@ThomasBreuer
Copy link
Contributor Author

I see three different reasons for the failures in package tests:

  1. The global objects SqrtField (packages corelg, nock) and QuantumField (package quagroup) have a Name value, which is no longer used in View output for algebras over these objects etc.; here we could add individual PrintObj methods as for Rationals etc.
  2. Individual objects created by package code get Name values, similar to the output of PrimitiveGroup, and the ViewObj method for something involving these objects calls PrintObj for them.
  3. There are Info calls in the tests of the crisp package, where individual objects with Name value occur as arguments.

I have no easy solution for the cases 2. and 3. At least some of the ViewObj methods belong to the package in question, but in fact many ViewObj methods in the GAP library call Print for subobjects. It would be more logical to call View in these situations, but I am afraid we do not really want to change all this.

@ThomasBreuer
Copy link
Contributor Author

For the moment, I give up:
A clean solution would require changes in a lot of ViewObj methods in the GAP library (and in packages) which call Print for certain subobjects.
I think the benefits are too small, compared to the efforts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: quirk Issues that are not bugs, but a discrepancy between user expectation and system behavior release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants