pretty printing for quotients, graded and localized multivariate polynomial rings #2184

simonbrandhorst · 2023-04-01T19:37:07Z

This pr should probably wait until the changes from nemo and abstract algebra propagate.

codecov · 2023-04-01T20:39:24Z

Codecov Report

Merging #2184 (31192c1) into master (d34b118) will decrease coverage by 71.94%.
The diff coverage is 0.00%.

❗ Current head 31192c1 differs from pull request most recent head 3f21812. Consider uploading reports for the commit 3f21812 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #2184       +/-   ##
==========================================
- Coverage   71.93%   0.00%   -71.94%     
==========================================
  Files         391     347       -44     
  Lines       52657   48392     -4265     
==========================================
- Hits        37879       0    -37879     
- Misses      14778   48392    +33614

Impacted Files	Coverage Δ
src/Rings/MPolyQuo.jl	`0.00% <0.00%> (-77.11%)`	⬇️
src/Rings/mpoly-graded.jl	`0.00% <0.00%> (-82.00%)`	⬇️

... and 423 files with indirect coverage changes

afkafkafk13 · 2023-04-04T09:14:57Z

src/Rings/MPolyQuo.jl

+ println(io, "Quotient")
+ println(io, " of ", base_ring(Q))
+ print(io, " by Ideal with " , ngens(modulus(Q)), "generators")


The key information of the quotient ring is the base_ring and the modulus.

I agree that in standard view only the number of generators is printed, but there should be an even more detailed view available, which prints the modulus (and not in supercompact printing of an ideal). Think of a user having defined several quotients of the same base_ring and trying to pick the right one for the next computation.

Some more information on the modulus is probably desirable in standard view.
There will be no more detailed view than standard view. In this case you have to ask the quotient ring for the information that you desire.

Indeed, this also solves the dilemma of how much information is necessary.

simonbrandhorst · 2023-05-25T08:19:26Z

src/Rings/localization_interface.jl

+function Base.show(io::IO, W::AbsLocalizedRing)
+ io = pretty(io)
+ if get(io, :supercompact, false)
+ print(io, "Localized ring")
+ else
+ print(io, "Localization of ", Lowercase(), base_ring(W))
+ print(pretty(IOContext(io, :supercompact =>true)), " at ", Lowercase(), inverted_set(W))
+ end


@thofma this is it.

thofma

A part from a few things, looks good from a technical point of view.

src/Rings/mpolyquo-localizations.jl

src/Rings/localization_interface.jl

src/Rings/mpoly-localizations.jl

afkafkafk13

I went through all the output and wrote at least one comment for each of the small nitpicks I have. Overall there are the following central points:

better pretty printing mult. set inverting powers of elements
something changed in try - catch doctests -- is this intentional
some supercompact suggestions

afkafkafk13 · 2023-05-26T06:37:35Z

docs/src/CommutativeAlgebra/GroebnerBases/groebner_bases.md

- x -> [1]
- y -> [2]
- z -> [3], MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}[x, y, z])
+(Graded multivariate polynomial ring in 3 variables over QQ, MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}[x, y, z])


minor nitpick: printing of the variables still needs improvement

'... over QQ, variables [x,y,z]'

What we print here is a tuple (Ring, Vector{Variables}) because the constructor also returns the variables.
Arrays always print their type. Since we do not own arrays (they come from Base), we cannot change this.
And it might not be a good idea even if we could.

I see.
What struck me, was the enormous disbalance between the amount of detail printed for the first entry (nice and suitable for mathematicians) and the second entry (which easily takes more than a line before even opening the square bracket, if you are dealing with a graded ring over an algebraic extension of QQ).

Something very similar has been discussed in #2064, although without a result.

For (multivarite) rings a solution would be to return the generators as a tuple and not as an array. The tuple does not print its type.

.... but this is not type stable, so we are back to arrays.

docs/src/CommutativeAlgebra/localizations.md

afkafkafk13 · 2023-05-26T06:47:20Z

experimental/Schemes/BlowupMorphism.jl

@@ -404,11 +404,11 @@ Spec of Multivariate polynomial ring in 3 variables over QQ

 =====================================
 Affine charts of the blown up scheme:
-Spec of Localization of Quotient of Multivariate polynomial ring in 3 variables over QQ by ideal(0, 0, 0) at the multiplicative set powers of QQMPolyRingElem[1]
+Spec of Localization of quotient of multivariate polynomial ring at multiplicative subset


I would suggest to be a bit more precise with the multiplicative set here, if this is not compact/supercompact: '... ring by inverting powers of an element' and suppress the multiplicative set completely in more compact printing. If we localize, we do it at a mult. closed subset, so the current information is redundant.

afkafkafk13 · 2023-05-26T06:52:26Z

src/AlgebraicGeometry/Schemes/AffineSchemes/Objects/Constructors.jl

@@ -39,7 +39,7 @@ julia> I = ideal(R, [x]);
 julia> U = complement_of_prime_ideal(I);

 julia> Spec(R, U)
-Spec of localization of Multivariate polynomial ring in 2 variables over QQ at the complement of ideal(x)
+Spec of Localization of multivariate polynomial ring in 2 variables over QQ at Complement of prime ideal


Do you really want this in capital letters? (Localization, Complement)

afkafkafk13 · 2023-05-26T06:54:39Z

src/AlgebraicGeometry/Schemes/ProjectiveSchemes/Objects/Attributes.jl

- x -> [1]
- y -> [1]
- z -> [1], MPolyDecRingElem{MPolyQuoRingElem{QQMPolyRingElem}, AbstractAlgebra.Generic.MPoly{MPolyQuoRingElem{QQMPolyRingElem}}}[x, y, z])
+(Graded multivariate polynomial ring in 3 variables over Quotient of multivariate polynomial ring, MPolyDecRingElem{MPolyQuoRingElem{QQMPolyRingElem}, AbstractAlgebra.Generic.MPoly{MPolyQuoRingElem{QQMPolyRingElem}}}[x, y, z])


minor nitpick: second and third argument still need pretty printing.

afkafkafk13 · 2023-05-26T07:04:23Z

src/Rings/PBWAlgebra.jl

@@ -1306,7 +1306,7 @@ julia> I = left_ideal(A, [p, q])
 left_ideal(p, q)

 julia> try eliminate(I, [q]); catch e; e; end # in fact, no elimination ordering exists
-ErrorException("could not find elimination ordering")
+


src/Rings/localization_interface.jl

src/Rings/mpoly-localizations.jl

afkafkafk13 · 2023-05-26T07:15:15Z

src/Rings/mpoly-localizations.jl

- print(io, "powers of ")
- print(io, denominators(S))
+ io = pretty(io)
+ if get(io, :supercompact, false)


In supercompact, either print nothing at all or something stating that we are dealing with powers/products of elements.
The 'multiplicative set' should be left for multiple localizations, where we see more general multiplicative sets (unless we simplify them, which is not even on our agenda...)

experimental/FTheoryTools/src/TateModels/constructors.jl

simonbrandhorst · 2023-05-26T13:19:39Z

src/Rings/mpoly-localizations.jl

+julia> PSI = hom(TL, RQL, RQL.([x]))
+Ring homomorphism
+ from localization of multivariate polynomial ring in 1 variable over QQ at complement of maximal ideal
+ to Localization of quotient of multivariate polynomial ring at complement of maximal ideal


@thofma the Localization should be lowercase ?

afkafkafk13 · 2023-05-26T14:48:48Z

src/Rings/mpoly-localizations.jl

+ from Localization of quotient of multivariate polynomial ring at complement of maximal ideal
+ to localization of multivariate polynomial ring in 1 variable over QQ at complement of maximal ideal


for MPolyQuo the localizations start with a capital letter, for MPolyRing with a lowercase letter. Probably there is still a typo in the MPolyQuo-case somewhere.

there is still something fishy in our printing system. @thofma is investigating

afkafkafk13

Looks good to me.
Remember to change the respective testset in mpoly-localizations.jl to agree with the new output format.

simonbrandhorst · 2023-05-26T14:55:21Z

Looks good to me. Remember to change the respective testset in mpoly-localizations.jl to agree with the new output format.

I removed these old tests and added some doctests instead.

afkafkafk13

Now this is a clear approval.

Sideremark: Finding and solving the 'something fishy' in the printing system is related to this PR, but not part of the changes.

afkafkafk13 reviewed Apr 4, 2023

View reviewed changes

simonbrandhorst commented May 25, 2023

View reviewed changes

simonbrandhorst requested review from afkafkafk13, thofma and jankoboehm May 25, 2023 20:12

simonbrandhorst marked this pull request as ready for review May 25, 2023 20:12

thofma reviewed May 25, 2023

View reviewed changes

src/Rings/mpolyquo-localizations.jl Show resolved Hide resolved

src/Rings/localization_interface.jl Outdated Show resolved Hide resolved

src/Rings/mpoly-localizations.jl Show resolved Hide resolved

src/Rings/mpoly-localizations.jl Outdated Show resolved Hide resolved

simonbrandhorst changed the title ~~pretting printing for quotients and graded multivariate polynomial rings~~ pretty printing for quotients and graded multivariate polynomial rings May 26, 2023

simonbrandhorst changed the title ~~pretty printing for quotients and graded multivariate polynomial rings~~ pretty printing for quotients, graded and localized multivariate polynomial rings May 26, 2023

afkafkafk13 reviewed May 26, 2023

View reviewed changes

simonbrandhorst added 3 commits May 26, 2023 14:01

show functions for graded quotient and localized MPolyRings

cf625c1

import pretty printing tool from AbstractAlgebra

d458a57

fix doctests

d3684de

afkafkafk13 reviewed May 26, 2023

View reviewed changes

experimental/FTheoryTools/src/TateModels/constructors.jl Outdated Show resolved Hide resolved

simonbrandhorst force-pushed the pretty_print_quo branch from 76f659f to d3684de Compare May 26, 2023 12:49

simonbrandhorst requested a review from afkafkafk13 May 26, 2023 12:50

simonbrandhorst commented May 26, 2023

View reviewed changes

bump AA to 0.30.04 and remove some old printing tests

9714e0f

afkafkafk13 reviewed May 26, 2023

View reviewed changes

remove some more old printing tests

3f21812

afkafkafk13 approved these changes May 26, 2023

View reviewed changes

thofma added 2 commits May 26, 2023 23:26

Bump AA version

458a4ec

some further fixes

35c59f3

thofma enabled auto-merge (squash) May 27, 2023 06:20

thofma merged commit 04820d1 into oscar-system:master May 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pretty printing for quotients, graded and localized multivariate polynomial rings #2184

pretty printing for quotients, graded and localized multivariate polynomial rings #2184

simonbrandhorst commented Apr 1, 2023

codecov bot commented Apr 1, 2023 •

edited

Loading

afkafkafk13 Apr 4, 2023

simonbrandhorst Apr 4, 2023 •

edited

Loading

afkafkafk13 Apr 4, 2023

simonbrandhorst May 25, 2023

thofma left a comment

afkafkafk13 left a comment

afkafkafk13 May 26, 2023

simonbrandhorst May 26, 2023

afkafkafk13 May 26, 2023 •

edited

Loading

simonbrandhorst May 26, 2023

afkafkafk13 May 26, 2023

lgoettgens May 26, 2023

simonbrandhorst May 26, 2023

simonbrandhorst May 27, 2023

afkafkafk13 May 26, 2023

afkafkafk13 May 26, 2023

afkafkafk13 May 26, 2023

afkafkafk13 May 26, 2023

afkafkafk13 May 26, 2023

simonbrandhorst May 26, 2023

afkafkafk13 May 26, 2023

simonbrandhorst May 26, 2023

afkafkafk13 left a comment

simonbrandhorst commented May 26, 2023

afkafkafk13 left a comment •

edited

Loading

		from Localization of quotient of multivariate polynomial ring at complement of maximal ideal
		to localization of multivariate polynomial ring in 1 variable over QQ at complement of maximal ideal

pretty printing for quotients, graded and localized multivariate polynomial rings #2184

pretty printing for quotients, graded and localized multivariate polynomial rings #2184

Conversation

simonbrandhorst commented Apr 1, 2023

codecov bot commented Apr 1, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

simonbrandhorst Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thofma left a comment

Choose a reason for hiding this comment

afkafkafk13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afkafkafk13 May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afkafkafk13 left a comment

Choose a reason for hiding this comment

simonbrandhorst commented May 26, 2023

afkafkafk13 left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Apr 1, 2023 •

edited

Loading

simonbrandhorst Apr 4, 2023 •

edited

Loading

afkafkafk13 May 26, 2023 •

edited

Loading

afkafkafk13 left a comment •

edited

Loading