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

Overhaul special printing macros #1594

Merged
merged 4 commits into from
Feb 6, 2024
Merged

Conversation

lgoettgens
Copy link
Collaborator

Inspired by oscar-system/Oscar.jl#3288 (comment).

This PR contains the following minor changes:

  • Document the behavior of the special printing macros and their helper functions.
  • Introduce get_name as an alternative to find_name in downstream packages, that does the same steps as the printing to find a name (e.g. first look into attribute storage before finding a new name in the global namespace). find_name should get changed to being private once all call-sites use get_name.

One fundamental change/question is the following:
It was motivated by oscar-system/Oscar.jl#3288 (comment), where we tried to enable @show_name for multivariate polynomial rings. The issue was that rings print the name that was first assigned to them (to be correct: the lexicographic first name at the moment when the first show function of the ring was called). This led to the following problem:
In Doctest1 (inside say DocModule1) we have R, _ = polynomial_ring(QQ, 3). Later, in Doctest2 (inside say DocModule2), we have S, _ = polynomial_ring(QQ, 3). As this object gets cached, it remembered its initial name R and prints as R even though it is only known as S in the current scope (DocModule2.R is not assigned).

Furthermore, the following is unintuitive:

julia> A = some_object_with_attribute_storage

julia> show(IOContext(:compact => true), A)
A

julia> B = A;

julia> A = 0;

julia> show(IOContext(:compact => true), B)
A

I would expect this now to be printed using the only valid name of the object B.

After talking to @thofma, there seems to be 3 ways how to handle name finding:

  1. Cache it the first time show is called. Current functionality, leads to the issue explained above.
  2. Don't cache anything (except some user / library code calls set_name! explicitly) and instead find a name to print each time it is needed. This is currently implemented in this PR. The only issue I can think of is that printing of an object (say in variable R) changes by executing A = R, as R now has the name A (again the lexicographic first one gets chosen).
  3. Cache it the first time show is called. When looking up a name from the cache, verify that the name still points to the same object. If yes, be happy and take this name. If not, find a new name and put this into the cache.

Before I spend more time on this, I would like to know which of these three is the wished functionality.

Pinging @fieker @fingolfin @thofma @joschmitt to leave their opinion on this here (as you either worked on or suffered due to this code).

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (f7a5678) 87.15% compared to head (a4b503a) 87.04%.

Files Patch % Lines
src/PrettyPrinting.jl 17.74% 51 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1594      +/-   ##
==========================================
- Coverage   87.15%   87.04%   -0.11%     
==========================================
  Files         115      115              
  Lines       29576    29593      +17     
==========================================
- Hits        25776    25760      -16     
- Misses       3800     3833      +33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joschmitt
Copy link
Collaborator

I would go for 2). Assuming that it doesn't take too much time to find out the name, I don't see a benefit from caching. Especially not, if one has to check every time if this is still the right name.

@lgoettgens
Copy link
Collaborator Author

I would go for 2). Assuming that it doesn't take too much time to find out the name, I don't see a benefit from caching. Especially not, if one has to check every time if this is still the right name.

Printing is relatively slow anyway, so this shouldn't make any notable difference.

@ fieker confirmed that 1) is a bad idea (see oscar-system/Oscar.jl#3288 (comment)).

So I will continue with 2). This PR is now ready from my POV. Everything still is non-breaking. Once this is released, I will update some things downstream that use this code (in a more or less wrong/weird way). And in the next breaking release, I would like to then do some technically breaking changes to this (that won't affect our projects due to the prior changes).

@fingolfin
Copy link
Member

Just a quick note, as a variant 3, one could "validated caching": that is, once a name has been found, cache it, but also store that this was a name determined via searching get_current_module() . Then when it is time to use, it, check whether isdefined(get_current_module(), thatName) -- if true then use it, if false, then clear it and proceed as if no name was set.

This has two advantages:

  1. it avoids some overhead,
  2. more importantly to me, it enhances stability of the name if the user assigns the object to multiple variables: i.f. if you do R = makeSomeObject() and then print, the name R is used. But if you later do S = R then which name will it use from here on, S or R ?

src/PrettyPrinting.jl Outdated Show resolved Hide resolved
AbstractAlgebra.set_name!
AbstractAlgebra.extra_name
AbstractAlgebra.find_name
```
Copy link
Member

Choose a reason for hiding this comment

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

What is really missing is IMHO a text that explains

  • how these things interact (in particular: what has precedence over what when it comes to determining a name)
  • how to use the macros in show functions (e.g. to place them first; in which order; possibly how they related to (supercompact), oneline and detailed printing)

That's not an objection to this PR which starts to untangle and document thing, documenting each function and macro certainly is already helpful; but it is still difficult (if it is possible at all) to get the full picture just from reading those. So we need something to tie it all together

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The precedence is in the docstring of get_name.
And examples of how to use everything are in the markdown doc page.

src/PrettyPrinting.jl Outdated Show resolved Hide resolved
src/PrettyPrinting.jl Outdated Show resolved Hide resolved
src/PrettyPrinting.jl Outdated Show resolved Hide resolved
src/PrettyPrinting.jl Outdated Show resolved Hide resolved
src/PrettyPrinting.jl Outdated Show resolved Hide resolved
src/PrettyPrinting.jl Show resolved Hide resolved
src/PrettyPrinting.jl Outdated Show resolved Hide resolved
src/PrettyPrinting.jl Outdated Show resolved Hide resolved
src/PrettyPrinting.jl Outdated Show resolved Hide resolved
@lgoettgens lgoettgens force-pushed the lg/show-name branch 2 times, most recently from 5b3d6a3 to d5a2065 Compare February 5, 2024 09:32
@fieker
Copy link
Contributor

fieker commented Feb 5, 2024

My 2 pennies:

  • what is the difference between find_name and get_name?
  • maybe explain what the point of extra_name is, e.g. in the case of free modules wihtout a name, when the ring has a name, it might generate R^n, or maximal order O_K of the field is named, or similar. It surrently is heavily under used.
    Shall we drop it? (different discussion)
  • I have no idea if the control flow now is as before (other than also checking for supercompact) or not
  • the named printing was always only going to be useful in interactive mode. It was never meant to be perfect - in particular as this is not possible
  • be my guest

@lgoettgens
Copy link
Collaborator Author

Just a quick note, as a variant 3, one could "validated caching": that is, once a name has been found, cache it, but also store that this was a name determined via searching get_current_module() . Then when it is time to use, it, check whether isdefined(get_current_module(), thatName) -- if true then use it, if false, then clear it and proceed as if no name was set.

isdefined(get_current_module(), thatName) is not enough to check as the variable could be used for something else. So it would be isdefined(get_current_module(), thatName) && get_current_module().thatName === obj

This has two advantages:

  1. it avoids some overhead,
  2. more importantly to me, it enhances stability of the name if the user assigns the object to multiple variables: i.f. if you do R = makeSomeObject() and then print, the name R is used. But if you later do S = R then which name will it use from here on, S or R ?

One change is that the name search is no longer called for every print, but only for compact and supercompact printing (to make it more performant). And in your case, it would choose the lexicographic first valid name for the object. So in your example it would keep R. But if you did S = someObject(), and later R = S, it would then choose R from that point on.

@lgoettgens
Copy link
Collaborator Author

My 2 pennies:

  • what is the difference between find_name and get_name?

find_name just searches the current namespace for a variable name. This is soon to be turned private.
get_name handles all of the precedence stuff between set_name!, find_name and extra_name. This is the function that should be called from printing code (if special printing is needed).

  • maybe explain what the point of extra_name is, e.g. in the case of free modules wihtout a name, when the ring has a name, it might generate R^n, or maximal order O_K of the field is named, or similar. It currently is heavily under used.
    Shall we drop it? (different discussion)

I'll add an example.

  • I have no idea if the control flow now is as before (other than also checking for supercompact) or not

Only a slight modification: We now first check for compact/supercompact properties before looking up a name. Without caching, this is more performant. Other than that, no changes

Co-authored-by: Max Horn <max@quendi.de>
@lgoettgens
Copy link
Collaborator Author

This is now updated to @fingolfin's wish with 3.
This now works as follows (using MPoly as an example, name printing is currently not enabled for it)

julia> R, x = polynomial_ring(QQ, 4)
(Multivariate polynomial ring in 4 variables over rationals, AbstractAlgebra.Generic.MPoly{Rational{BigInt}}[x1, x2, x3, x4])

julia> show(IOContext(stdout, :compact => true), R)
R
julia> A = R
Multivariate polynomial ring in 4 variables x1, x2, x3, x4
  over rationals

julia> show(IOContext(stdout, :compact => true), R)
R
julia> show(IOContext(stdout, :compact => true), A)
R
julia> R = 4
4

julia> show(IOContext(stdout, :compact => true), A)
A

@fingolfin is this as you want it?

@fingolfin
Copy link
Member

@lgoettgens that looks good (and yes I forget the check whether the variable content is still identical to the object -- but you guessed my intention correctly and compensated for my mistake, thank you :-) )

@lgoettgens
Copy link
Collaborator Author

@lgoettgens that looks good (and yes I forget the check whether the variable content is still identical to the object -- but you guessed my intention correctly and compensated for my mistake, thank you :-) )

I'll take this as approval.

@lgoettgens
Copy link
Collaborator Author

lgoettgens commented Feb 5, 2024

The succeeding matching CI verifies that oscar-system/Oscar.jl#3318 adapts Oscar for this PR to be non-breaking.

@lgoettgens lgoettgens closed this Feb 6, 2024
@lgoettgens lgoettgens reopened this Feb 6, 2024
@lgoettgens lgoettgens merged commit b85f5d0 into Nemocas:master Feb 6, 2024
26 of 28 checks passed
@lgoettgens lgoettgens deleted the lg/show-name branch February 6, 2024 17:08
ooinaruhugh pushed a commit to ooinaruhugh/AbstractAlgebra.jl that referenced this pull request Feb 15, 2024
Co-authored-by: Max Horn <max@quendi.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants