Yet another attempt to revise printing for modules #4251

fingolfin · 2024-10-28T13:00:35Z

See #3199 for an earlier version of this. It tries to take concerns from that PR into account.

I am sure this is not perfect, but IMHO superior in almost every way to the status quo.

lgoettgens · 2024-10-28T14:15:33Z

This still uses a lot of setting and checking properties of some IOContexts. Didn't we introduce terse(io) and is_terse(io) to not have to do that anymore?

fingolfin · 2024-10-28T17:12:57Z

Unfortunately terse mode is not sufficient on its own, we still need compact. In particular when objects have a "name" then often "compact" prints more compactly than "terse" mode (as it prints the name). I've suggested before that perhaps the @show_name should also trigger name printing when "terse" mode is on, but currently that is not the case.

So the current use of IOContext was carefully tuned to ensure printing does not regress compared to what was before. In fact we now print object "names" in a few more cases than before.

HechtiDerLachs

Unfortunately, I am still not an expert on the internals of the printing. And as I recall, @wdecker and @jankoboehm had some rather explicit ideas on how things should look in the REPL. But other than that, this seems good from my point of view. Thank you for looking into this!

HechtiDerLachs

Judging from the changes of the doctests, everything looks much better imho.

lgoettgens · 2024-10-28T14:11:47Z

src/Modules/ModulesGraded.jl

  print(io, "Decorated free module of rank $(rank(F)) over ")
-  print(IOContext(io, :compact =>true), base_ring(F))
+  print(IOContext(io, :compact => true), base_ring(F))


isn't this missing a Lowercase() in front of the base ring?

Probably (and then also io = pretty(io).

But this code is very fishy to start with: the code goes on in a loop which prints the base ring again, without a separator.

There is only one doctest exercising it but it shows the problem (from src/Modules/ModulesGraded.jl:2049)

julia> R, (x,y) = graded_polynomial_ring(QQ, [:x, :y]) (Graded multivariate polynomial ring in 2 variables over QQ, MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}[x, y]) julia> free_module_dec(R,3) Decorated free module of rank 3 over RR^3([0])

Note how it prints RR^3([0]).

That looks like a genuine bug -- but it predates this PR and is already a bug on master. So I'd rather not touch this code further and leave it to people using this code to decide how to fix it. (Maybe nobody uses it, though)

ederc · 2024-10-28T18:15:00Z

Why do we change from

1 -> x*e[1]
2 -> y*e[1]

to

  1: x*e[1]
  2: y*e[1]

? The printing for ideal generators uses -> (with indentation). I do not have any preference, but shouldn't we use the "same" style of printing module and ideal generators?

wdecker · 2024-10-28T21:18:54Z

? The printing for ideal generators uses -> (with indentation). I do not have any preference, but shouldn't we use the "same" style of printing module and ideal generators?

I agree with @ederc that we should aim at unification here. But even for ideals, this is not achieved yet:

julia> R, (x,y) = QQ["x","y"];

julia> I = ideal(R, [x, y])
Ideal generated by
x
y

julia> gens(I)
2-element Vector{QQMPolyRingElem}:
x
y

julia> groebner_basis(I)
Gröbner basis with elements
1 -> y
2 -> x
with respect to the ordering
degrevlex([x, y])

wdecker · 2024-10-28T21:21:19Z

Sorry, in my last remark indentations are not shown.

fingolfin · 2024-10-28T23:27:01Z

I used 1: x instead of 1 -> x because the latter looks like a mapping to me (and is in fact used like that when printing mappings).

I agree that printing of ideals also should be unified. Thus I've changed it to also : instead of ->. I am sure there are more things to be aligned, but arguably fewer with this PR than right now on master...

codecov · 2024-10-29T03:01:19Z

Codecov Report

Attention: Patch coverage is 88.88889% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.59%. Comparing base (b056c72) to head (6353b72).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Modules/UngradedModules/FreeModuleHom.jl	78.26%	5 Missing ⚠️
src/Modules/UngradedModules/SubquoModule.jl	75.00%	3 Missing ⚠️
src/Modules/UngradedModules/ModuleGens.jl	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4251      +/-   ##
==========================================
- Coverage   84.60%   84.59%   -0.01%     
==========================================
  Files         640      640              
  Lines       85122    85126       +4     
==========================================
+ Hits        72014    72015       +1     
- Misses      13108    13111       +3

Files with missing lines	Coverage Δ
experimental/GroebnerWalk/src/common.jl	`34.61% <ø> (ø)`
experimental/Schemes/src/Tjurina.jl	`100.00% <ø> (ø)`
...gebraicGeometry/Schemes/Sheaves/CoherentSheaves.jl	`80.80% <ø> (ø)`
...ry/Surfaces/AdjunctionProcess/AdjunctionProcess.jl	`93.10% <ø> (ø)`
src/Modules/ModuleTypes.jl	`78.90% <ø> (ø)`
src/Modules/ModulesGraded.jl	`73.74% <100.00%> (+0.02%)`	⬆️
src/Modules/UngradedModules/FreeMod.jl	`87.69% <100.00%> (+0.09%)`	⬆️
src/Modules/UngradedModules/FreeResolutions.jl	`81.38% <100.00%> (+0.08%)`	⬆️
src/Modules/UngradedModules/Hom_and_ext.jl	`98.46% <ø> (ø)`
src/Modules/UngradedModules/HomologicalAlgebra.jl	`92.81% <ø> (ø)`
... and 20 more

joschmitt · 2024-10-29T08:39:05Z

The changes to the ideal printing created a conflict with #4250 unfortunately.

fingolfin · 2024-10-29T08:58:18Z

The conflict should be easy to resolve, but before I invest that work I'll wait to hear what people think of the changes.

wdecker · 2024-10-29T12:21:00Z

src/Modules/UngradedModules/FreeModuleHom.jl

-=========
-Free module of rank 3 over R)
+  1: x*z*e[1] - y*z*e[2] + y^2*e[3]
+represented as subquotient with no relations, Hom: submodule with 1 generator


Why is Hom written with capitel H? It is not meant to be the module Hom, it is meant to be a homomorphisms.

this is to be consistent with all other homomorphisms in OSCAR

We just discussed this here. To summarize: the , comes from Julia printing a tuple. The second tuple element is a homomorphism. And by our conventions, when we print objects then we capitalize "at the beginning" as if it was a sentence. Of course that's a bit jarring here, because Hom: is not at the start of a line, but rather comes after a comma. But I see no good way to overcome this.

Note that there are plenty of existing doctests that look just like this, e.g.:

julia> reduced_scheme(X) (scheme(x^2 - 2*x*y + y^2, x - y), Hom: scheme(x^2 - 2*x*y + y^2, x - y) -> scheme(x^2 - 2*x*y + y^2))

or from invariant theory

julia> A, AtoS = affine_algebra(RG) (Quotient of multivariate polynomial ring by ideal (0), Hom: A -> S)

The example here is perhaps worse in so far as the context suggest that one might be talking about a Hom-module. But then I hope that the notation Hom: submodule ... eventually makes it clear enough.

I understand, but for the working mathematician this is hard to crasp and very confusing. It is getting even worth: Look at this (old) convention:

julia> V, f = hom(F1, F2)
(hom of (F1, F2), Map: V -> set of all homomorphisms from F1 to F2)

So here, the hom should read Hom and the Map should read map:

Hom(F1, F2), map V -> Hom(F1, F2)

To make things consistent could also mean that we change the other stuff as well? Why starting with a capital letter after a comma?

To make things consistent could also mean that we change the other stuff as well? Why starting with a capital letter after a comma?

The comma is there since julia prints a tuple by printing each element of the tuple comma-separated. From the print-call to our object, we have no way to detect if we are called from inside a tuple-printing or somewhere else. So this is essentially due to the way the julia printing works

Yes, sure, we have to make a choice and use this choice as an abbreviation for homomorphism wherever appropriate. But again, using Hom as abbreviation is confusing. Anyway, I give up at this point. Otherwise, everything looks much better as before. Thx, @fingolfin.

I am happy to have Hom: changed to something else (but what?). But IMHO it should be done for all (homo)morphisms, not just module homomorphisms. I.e. I think this should be a separate discussion.

wdecker · 2024-10-29T17:02:35Z

Yes, postpone and merge this PR as soon as ready otherwise.Von meinem iPhone gesendetAm 29.10.2024 um 17:22 schrieb Max Horn ***@***.***>: @fingolfin commented on this pull request. In src/Modules/UngradedModules/FreeModuleHom.jl:

@@ -427,16 +425,10 @@ julia> a = hom(F, G, V);

julia> kernel(a) (Submodule with 1 generator -1 -> x*z*e[1] - y*z*e[2] + y^2*e[3] -represented as subquotient with no relations., Map with following data -Domain: -======= -Submodule with 1 generator -1 -> x*z*e[1] - y*z*e[2] + y^2*e[3] -represented as subquotient with no relations. -Codomain: -========= -Free module of rank 3 over R) + 1: x*z*e[1] - y*z*e[2] + y^2*e[3] +represented as subquotient with no relations, Hom: submodule with 1 generator I am happy to have Hom: changed to something else (but what?). But IMHO it should be done for all (homo)morphisms, not just module homomorphisms. I.e. I think this should be a separate discussion. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

* Revise printing of modules * Adjust test case in test/Modules/ExteriorPowers.jl for new printing * update jldoctest * Also adjust printing of ideals * update doctests * Also update README.md * Update code snippet, change to jldoctest * Update code snippets which are not doctests * Update booktests

fingolfin requested review from ederc, jankoboehm, wdecker and HechtiDerLachs October 28, 2024 13:00

HechtiDerLachs reviewed Oct 28, 2024

View reviewed changes

HechtiDerLachs approved these changes Oct 28, 2024

View reviewed changes

lgoettgens reviewed Oct 28, 2024

View reviewed changes

fingolfin force-pushed the mh/show-UngradedModules-new branch from c07626a to c46602c Compare October 28, 2024 23:30

fingolfin force-pushed the mh/show-UngradedModules-new branch from c46602c to d7b3595 Compare October 29, 2024 07:40

ederc approved these changes Oct 29, 2024

View reviewed changes

wdecker reviewed Oct 29, 2024

View reviewed changes

fingolfin added 9 commits October 29, 2024 15:36

Revise printing of modules

116a59c

Adjust test case in test/Modules/ExteriorPowers.jl for new printing

4543ee2

update jldoctest

922b87d

Also adjust printing of ideals

fadff29

update doctests

38e5d36

Also update README.md

9d5e452

Update code snippet, change to jldoctest

349f909

Update code snippets which are not doctests

b0991ac

Update booktests

6353b72

fingolfin force-pushed the mh/show-UngradedModules-new branch from d7b3595 to 6353b72 Compare October 29, 2024 15:14

fingolfin merged commit 84ad208 into oscar-system:master Oct 29, 2024
29 checks passed

fingolfin deleted the mh/show-UngradedModules-new branch October 29, 2024 18:08

lgoettgens mentioned this pull request Nov 6, 2024

Improve printing of sub and quo modules, graded and ungraded cases #2704

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yet another attempt to revise printing for modules #4251

Yet another attempt to revise printing for modules #4251

fingolfin commented Oct 28, 2024

lgoettgens commented Oct 28, 2024

fingolfin commented Oct 28, 2024

HechtiDerLachs left a comment

HechtiDerLachs left a comment

lgoettgens Oct 28, 2024

fingolfin Oct 28, 2024

ederc commented Oct 28, 2024

wdecker commented Oct 28, 2024

wdecker commented Oct 28, 2024

fingolfin commented Oct 28, 2024

codecov bot commented Oct 29, 2024 •

edited

Loading

joschmitt commented Oct 29, 2024

fingolfin commented Oct 29, 2024

wdecker Oct 29, 2024

lgoettgens Oct 29, 2024

fingolfin Oct 29, 2024

wdecker Oct 29, 2024

lgoettgens Oct 29, 2024

wdecker Oct 29, 2024

fingolfin Oct 29, 2024

wdecker commented Oct 29, 2024 via email

Yet another attempt to revise printing for modules #4251

Yet another attempt to revise printing for modules #4251

Conversation

fingolfin commented Oct 28, 2024

lgoettgens commented Oct 28, 2024

fingolfin commented Oct 28, 2024

HechtiDerLachs left a comment

Choose a reason for hiding this comment

HechtiDerLachs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ederc commented Oct 28, 2024

wdecker commented Oct 28, 2024

wdecker commented Oct 28, 2024

fingolfin commented Oct 28, 2024

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

joschmitt commented Oct 29, 2024

fingolfin commented Oct 29, 2024

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

wdecker commented Oct 29, 2024 via email

codecov bot commented Oct 29, 2024 •

edited

Loading