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

Tweak Betti table printing #4256

Merged

Conversation

fingolfin
Copy link
Member

This makes it look a bit nicer (which is subjective) but also ensure that the Betti tables in jldoctests are formatted such that doctest=:fix works correctly (it didn't before because the first printed line had too many leading spaces, confusing Documenter's code for detecting were an input line ends).

@jankoboehm
Copy link
Contributor

The : is what is found in most books and papers (mostly due to M2), but if the causes trouble I can live with |, but we should discuss that.

@fingolfin
Copy link
Member Author

I don't care about : vs. | much (although I find : weird for a table, but shrug), it is trivial to change.

My main interest is to get rid of the leading spaces in the first row of the table. We discussed this before, and as a temporary workaround the offending tests were changed to jldoctest to julia.

So far nobody has shown any signs of implementing a better fix that allows us to re-enable those tests, and so I am doing that now. As you can see in the diff, there were already test outputs that had started to diverge from what actually happens in OSCAR, and we didn't notice.

@fingolfin
Copy link
Member Author

I've updated this now to make a minimal change, i.e. it just inserts : in the first row.

As a side effect this also makes it even more obvious how much those examples have diverged since they were added

@fingolfin fingolfin force-pushed the mh/tweak-betti-table-printing branch from 3abdba4 to a2c3f67 Compare October 30, 2024 15:09
@wdecker
Copy link
Collaborator

wdecker commented Oct 30, 2024

@fingolfin Thx for doing this. I would like to discuss details in person. I will be at the university tomorrow.

@wdecker
Copy link
Collaborator

wdecker commented Oct 30, 2024

Here is a more extensive example for testing:

S, x = graded_polynomial_ring(QQ, :x => 1:14, [-1,-1,1,1,1,1,1,1,1,1,1,1,2,2]);
I = ideal(S, gens(S));
FI = free_resolution(I)
betti(FI)

@fingolfin
Copy link
Member Author

So that renders like this (if your terminal is wide enough):

julia> S, x = graded_polynomial_ring(QQ, :x => 1:14, [-1,-1,1,1,1,1,1,1,1,1,1,1,2,2]);

julia> I = ideal(S, gens(S));

julia> FI = free_resolution(I)
Free resolution of I
S^14 <---- S^91 <---- S^364 <---- S^1001 <---- S^2002 <---- S^3003 <---- S^3432 <---- S^3003 <---- S^2002 <---- S^1001 <---- S^364 <---- S^91 <---- S^14 <---- S^1 <---- 0
0          1          2           3            4            5            6            7            8            9            10          11         12         13        14

julia> betti(FI)
     :  0   1    2     3     4     5     6     7     8     9   10  11  12  13
-----------------------------------------------------------------------------
-3   :  -   1   10    45   120   210   252   210   120    45   10   1   -   -
-2   :  -   -    2    20    90   240   420   504   420   240   90  20   2   -
-1   :  2  20   90   241   430   549   540   450   342   230  122  45  10   1
0    :  -   4   40   180   480   840  1008   840   480   180   40   4   -   -
1    : 10  45  122   230   342   450   540   549   430   241   90  20   2   -
2    :  2  20   90   240   420   504   420   240    90    20    2   -   -   -
3    :  -   1   10    45   120   210   252   210   120    45   10   1   -   -
-----------------------------------------------------------------------------
total: 14  91  364  1001  2002  3003  3432  3003  2002  1001  364  91  14  1

@fingolfin
Copy link
Member Author

@jankoboehm and @wdecker and myself discussed this and we agreed on this layout:

julia> betti(FI)
degree:  0   1    2     3     4     5     6     7     8     9   10  11  12  13
------------------------------------------------------------------------------
    -3:  -   1   10    45   120   210   252   210   120    45   10   1   -   -
    -2:  -   -    2    20    90   240   420   504   420   240   90  20   2   -
    -1:  2  20   90   241   430   549   540   450   342   230  122  45  10   1
     0:  -   4   40   180   480   840  1008   840   480   180   40   4   -   -
     1: 10  45  122   230   342   450   540   549   430   241   90  20   2   -
     2:  2  20   90   240   420   504   420   240    90    20    2   -   -   -
     3:  -   1   10    45   120   210   252   210   120    45   10   1   -   -
------------------------------------------------------------------------------
 total: 14  91  364  1001  2002  3003  3432  3003  2002  1001  364  91  14   1

Copy link
Collaborator

@wdecker wdecker left a comment

Choose a reason for hiding this comment

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

Fine with me as soon as tests are green. @fingolfin Thanks again for dong this.

This makes it look a bit nicer (which is subjective) but also
ensure that the Betti tables in jldoctests are formatted such
that "doctest=:fix" works correctly (it didn't before because
the first printed line had too many leading spaces, confusing
Documenter's code for detecting were an input line ends).
@fingolfin fingolfin force-pushed the mh/tweak-betti-table-printing branch from a2c3f67 to 14f1a85 Compare November 7, 2024 11:47
@fingolfin
Copy link
Member Author

I've implemented the new styling and also changed on test to make it easier to maintain. Wolfram's example is now printed exactly as we discussed.

@fingolfin fingolfin enabled auto-merge (squash) November 7, 2024 14:57
@jankoboehm
Copy link
Contributor

I guess auto-merge has a problem with the book tests since the printing changed.

@benlorenz
Copy link
Member

benlorenz commented Nov 8, 2024

I guess auto-merge has a problem with the book tests since the printing changed.

I don't see any errors from betti tables in the log, both book test jobs (1.10 and nightly) were stuck in vinberg_2.jlcon for about 2 hours and then killed. I restarted them now.

Edit: Looking at the order of the chapters, I think the booktests will indeed fail later due to the changed printing.

@fingolfin
Copy link
Member Author

Ah I forgot about the book tests. Do we have something like Oscar.doctest_fix for the book test files to automatically update them?

Copy link

codecov bot commented Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.48%. Comparing base (24711ee) to head (b0db19c).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4256      +/-   ##
==========================================
- Coverage   84.49%   84.48%   -0.02%     
==========================================
  Files         641      641              
  Lines       85476    85539      +63     
==========================================
+ Hits        72226    72269      +43     
- Misses      13250    13270      +20     
Files with missing lines Coverage Δ
src/Modules/ModulesGraded.jl 73.49% <100.00%> (-0.05%) ⬇️
src/Modules/UngradedModules/FreeResolutions.jl 81.38% <ø> (ø)
src/Rings/mpoly-graded.jl 91.58% <ø> (ø)

... and 16 files with indirect coverage changes

@fingolfin fingolfin merged commit 194d82b into oscar-system:master Nov 8, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants