Fix vinberg booktests #3939

simonbrandhorst · 2024-07-15T14:59:19Z

This removes the file vinberg_3.jlcon and copies its contents to vinberg_2.jl a minimal fix is attempted to make it run with Oscar 1.1.
The booktests are also enabled now.

Edit: resolves #3961

simonbrandhorst · 2024-07-15T15:05:25Z

Cc: @HechtiDerLachs

simonbrandhorst · 2024-07-15T15:08:29Z

test/book/specialized/brandhorst-zach-fibration-hopping/vinberg_2.jlcon

@@ -122,17 +122,17 @@ julia> g, phi1 = two_neighbor_step(Y1, fibers[2][I]); g

 julia> kt2 = base_ring(parent(g)); P = kt2.([0,0]);

-julia> Y2, phi2 = elliptic_surface(g, P); Y2
+julia> Y2, phi2 = elliptic_surface(g, P; minimize=false); Y2


not sure in which Oscar versions this works. It is a recent addition. in 5dcfffb

I think it should work in 1.1.1, but not in 1.1.0.

codecov · 2024-07-15T15:53:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.33%. Comparing base (810a77d) to head (5f00b70).
Report is 34 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3939      +/-   ##
==========================================
+ Coverage   83.99%   84.33%   +0.34%     
==========================================
  Files         591      593       +2     
  Lines       81387    81731     +344     
==========================================
+ Hits        68360    68928     +568     
+ Misses      13027    12803     -224

Files	Coverage Δ
experimental/Schemes/src/elliptic_surface.jl	`91.23% <100.00%> (+3.97%)`	⬆️

... and 48 files with indirect coverage changes

simonbrandhorst · 2024-07-15T17:33:42Z

Locally on 1.2-Dev the booktest goes through quickly. On 1.10 it seemed to hang.
Locally on 1.11 they seem to work (at least no errors)

simonbrandhorst · 2024-07-16T07:12:52Z

@benlorenz would it be possible to run the book CI in 1.1.1? So far they are run in 1.1.0.
The difference between 1.1.0 and 1.1.1 are precisely two PRs about elliptic surfaces.
Cc. @fingolfin

benlorenz · 2024-07-16T07:27:21Z

The book tests for a PR are always run with the code from the corresponding branch. There is no 1.1.x involved. Also they are only run for 1.10 and not 1.11.

There are some output changes for the vinberg_2 file here:
https://github.com/oscar-system/Oscar.jl/actions/runs/9941759084/job/27468895215?pr=3939#step:9:14649
(and some more further down)

According to the timestamps this file took about 22 minutes in the CI, how long does it take locally and what are the memory requirements?

Mon, 15 Jul 2024 17:58:17 GMT    vinberg_2.jlcon
...
Mon, 15 Jul 2024 18:20:50 GMT specialized/brandhorst-zach-fibration-hopping: Test Failed at /home/runner/work/Oscar.jl/Oscar.jl/test/book/test.jl:248
Mon, 15 Jul 2024 18:20:50 GMT   Expression: normalize_repl_output(content) == computed
Mon, 15 Jul 2024 18:20:50 GMT    Evaluated: "julia> K = QQ;\n\njulia> Kt, t = polynomial_ring(K, :t);\n\njulia> Ktf = fraction_field(Kt);\n\njulia> E = ...

The vinberg_3 file is still mentioned in the json file which contains the ordering of the examples:
https://github.com/oscar-system/Oscar.jl/blob/sb/fix_bookchapter/test/book/ordered_examples.json#L234

HechtiDerLachs

The recent changes and the new constructor for elliptic surfaces require the tests for the book chapter to be adjusted. The changes in the code here seem rather minimal. Most of the new lines come from the changes of output in the actual tests.

If it works, the changes of this PR should be fine.

HechtiDerLachs · 2024-07-16T10:01:19Z

test/book/specialized/brandhorst-zach-fibration-hopping/vinberg_2.jlcon

@@ -122,17 +122,17 @@ julia> g, phi1 = two_neighbor_step(Y1, fibers[2][I]); g

 julia> kt2 = base_ring(parent(g)); P = kt2.([0,0]);

-julia> Y2, phi2 = elliptic_surface(g, P); Y2
+julia> Y2, phi2 = elliptic_surface(g, P; minimize=false); Y2


I think it should work in 1.1.1, but not in 1.1.0.

simonbrandhorst · 2024-07-17T07:37:50Z

Locally the tests run in about 15 minutes. Hence the 22 minutes in the CI seem realistic enough.
I fixed the booktests. The output changed, but all changes are mathematically equivalent.

The file is now changed in such a way that it will run without error and produce mathematically correct output.
However the output may and probably will fluctuate. The reason is that there is no canonical way to order the divisors, and Oscar just makes a choice without any guarantees.

benlorenz · 2024-07-17T09:22:35Z

Locally the tests run in about 15 minutes. Hence the 22 minutes in the CI seem realistic enough. I fixed the booktests. The output changed, but all changes are mathematically equivalent.

This almost doubles the time for the booktests, but as long they finish under 1 hour I think this should be ok.

The file is now changed in such a way that it will run without error and produce mathematically correct output. However the output may and probably will fluctuate. The reason is that there is no canonical way to order the divisors, and Oscar just makes a choice without any guarantees.

How are these choices made? We do try to seed all the random number generators for the booktests and in most other tests we do get stable output even with randomness involved in the algorithms.

simonbrandhorst · 2024-07-17T09:57:35Z

Some printing depends on the ordering of collect(keys(::IdDict) which is non-deterministic.
This comes up in the tests now.
And some choice of a vector space basis depends on the order that ideals are returned in a primary decomposition which comes from singular. Again no guarantees on the ordering of the components is given by Oscar.

At this point my impression is that this book chapter is just not suitable for a doctest. It was written to be interesting for the reader and not to be a sensible doctest.

just disable the test (20 minutes seems way too long for a good CI test??)
test something that actually makes sense and not the printing. But the .jlcon file will be (slightly) different from the book. E.g. we will not print certain lines whose output is not going to be deterministic but show them in the book nevertheless.

@fingolfin
I would suggest to go by 2), do some minimal changes to the code in the book as in this pr, put 2 footnotes in the book:
a) This code requires Oscar 1.1.1.
b) The choice of a basis may change and therefore so will some outputs.

fingolfin · 2024-07-17T10:23:08Z

test/book/specialized/brandhorst-zach-fibration-hopping/vinberg_2.jlcon

 given as the formal sum of
+  2 * component E8_3 of fiber over (0, 1)


If the order here keeps changing, that suggests a missing hash method for somewhere; well, most likely for the keys (resp. the type of the keys) in some dictionary. Is there a dict being used here? What is the type of the keys?

The type of the dictionary is
IdDict{Oscar.AbsIdealSheaf, ZZRingElem}

so AbsIdealSheaf is an abstract type, but all its subtypes seem to end with IdealSheaf. Alas git grep hash.*IdealSheaf reveals zero hits, i.e. no hashing methods for any of those types. At the same time, there is an equality method in experimental/Schemes/src/IdealSheaves.jl:466:

function ==(I::AbsIdealSheaf, J::AbsIdealSheaf) I === J && return true X = space(I) X == space(J) || return false for U in basic_patches(default_covering(X)) is_subset(I(U), J(U)) && is_subset(J(U), I(U)) || return false end return true end

But the rules are clear: when you define a custom == method you have to ensure hash still satisfies the implication if a==b then hash(a)==hash(b) which usually means a custom hash method has to be provided.

The absolute minimal "always correct" hash function would be this:

Base.hash(x::AbsIdealSheaf, h::UInt) = h

So you could try adding this, perhaps with a comment # TODO: replace by something better

The drawback of such a hash function is that it makes lookups of a key in a Dict slower. The upside is that it also makes it less wrong, which I think is overall preferable in our business... ;-)

Aha, but you are actually storing things in an IdDict -- so in that case the hash method does not matter at all.

But also: you have zero control over the order, because now instead of hash it uses objectid which essentially is the address of the object, and that of course can change when you re-run the program.

In that case the only fix I can think of is to modify your printing code to "sort" the keys in some way, based on some parameters -- doesn't have to be perfect (i.e. not a total order), just good enough to stabilize the output for this example.

In this example, perhaps one could "cheaply sort" the entries so that those with a "component" in the output string come (for example) first; and then those in turn are sorted on these strings E8_4 etc.; and when there is a tie, sort next by the points they are a fiber over (i.e. (0,1) and so on?

(Or switch to a Dict after adding a suitable hash function -- but perhaps your == method is too slow for that?)

BTWH hash(h) = h will still not give fully stable output, but assuming your algorithm is deterministic, or at least runs with a fixed RNG seed (as is the case here) the order should be stable.

As you guessed we chose IdDict because == is prohibitively expensive

thofma · 2024-07-19T12:07:45Z

P.S.: The CI error is some unrelated (still newish) bug exposed in Schemes test code, see #3947.

simonbrandhorst · 2024-07-23T09:25:13Z

I suppressed printing of the indeterministic parts. The latest testrun reported:
specialized/brandhorst-zach-fibration-hopping | 2 2 17m51.3s

But somehow the tests timed out anyways.

simonbrandhorst · 2024-07-23T11:36:04Z

The booktests seem to die in garbage collection. No clue what is going on.

joschmitt · 2024-07-23T11:47:59Z

The booktests seem to die in garbage collection. No clue what is going on.

I think this was a known error that happens sometimes; I can't find the issue right now though. I restarted the CI job.

joschmitt · 2024-07-23T12:44:19Z

Now the actual book tests seem to have passed, but THEN there was some error?

/workspace/srcdir/mongo-c-driver/src/libmongoc/src/mongoc/mongoc-ocsp-cache.c:158 _mongoc_ocsp_cache_get_status(): precondition failed: pthread_mutex_lock ((&ocsp_cache_mutex)) == 0

[1965] signal (6.-6): Aborted

benlorenz · 2024-07-23T13:29:11Z

Now the actual book tests seem to have past, but THEN there was some error?

/workspace/srcdir/mongo-c-driver/src/libmongoc/src/mongoc/mongoc-ocsp-cache.c:158 _mongoc_ocsp_cache_get_status(): precondition failed: pthread_mutex_lock ((&ocsp_cache_mutex)) == 0

[1965] signal (6.-6): Aborted

That looks like an error when running the finalizers for the mongodb driver (that is used for polydb access in some other booktests), I don't remember having seen this previously.
I will look into this.

* booktest: explicitly destroy mongodb db and client * booktest-polydb-cleanup: try nicer cleanup

benlorenz · 2024-07-23T20:08:57Z

I merged #3963 into this PR here which fixes the mongoc error and the tests are all green now.

simonbrandhorst · 2024-07-24T06:59:43Z

Thank you :-) @benlorenz @joschmitt

This reverts commit b04ecbb.

* fix booktests for vinberg_3.jlcon and move it to vinberg_2.jlcon * remove vinberg from ordered_examples.json * fix tests * suppress printing of unstable outputs * booktest: explicitly destroy mongodb db and client (oscar-system#3963) * booktest: explicitly destroy mongodb db and client * booktest-polydb-cleanup: try nicer cleanup --------- Co-authored-by: Benjamin Lorenz <[email protected]>

simonbrandhorst mentioned this pull request Jul 15, 2024

Elliptic fibration book example errors #3881

Closed

simonbrandhorst commented Jul 15, 2024

View reviewed changes

fix booktests for vinberg_3.jlcon and move it to vinberg_2.jlcon

942bb3b

lgoettgens added the oscar book PRs necessary for the Oscar book label Jul 15, 2024

HechtiDerLachs approved these changes Jul 16, 2024

View reviewed changes

remove vinberg from ordered_examples.json

0870a49

fix tests

a913f80

fingolfin reviewed Jul 17, 2024

View reviewed changes

suppress printing of unstable outputs

8d48266

lgoettgens mentioned this pull request Jul 23, 2024

Mongodb error after running booktests #3961

Closed

benlorenz mentioned this pull request Jul 23, 2024

booktest: explicitly destroy mongodb db and client #3963

Merged

booktest: explicitly destroy mongodb db and client (#3963)

5f00b70

* booktest: explicitly destroy mongodb db and client * booktest-polydb-cleanup: try nicer cleanup

joschmitt added the triage label Jul 24, 2024

fingolfin approved these changes Jul 24, 2024

View reviewed changes

fingolfin merged commit b04ecbb into master Jul 24, 2024
30 checks passed

fingolfin deleted the sb/fix_bookchapter branch July 24, 2024 09:36

fingolfin added a commit that referenced this pull request Jul 24, 2024

Revert "Fix vinberg booktests (#3939)"

0e46a72

This reverts commit b04ecbb.

joschmitt removed the triage label Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix vinberg booktests #3939

Fix vinberg booktests #3939

simonbrandhorst commented Jul 15, 2024 •

edited by lgoettgens

Loading

simonbrandhorst commented Jul 15, 2024

simonbrandhorst Jul 15, 2024

HechtiDerLachs Jul 16, 2024

codecov bot commented Jul 15, 2024 •

edited

Loading

simonbrandhorst commented Jul 15, 2024 •

edited

Loading

simonbrandhorst commented Jul 16, 2024

benlorenz commented Jul 16, 2024

HechtiDerLachs left a comment

HechtiDerLachs Jul 16, 2024

simonbrandhorst commented Jul 17, 2024

benlorenz commented Jul 17, 2024

simonbrandhorst commented Jul 17, 2024 •

edited

Loading

fingolfin Jul 17, 2024

simonbrandhorst Jul 17, 2024

fingolfin Jul 17, 2024

fingolfin Jul 17, 2024

fingolfin Jul 17, 2024

fingolfin Jul 17, 2024

simonbrandhorst Jul 17, 2024

thofma commented Jul 19, 2024

simonbrandhorst commented Jul 23, 2024

simonbrandhorst commented Jul 23, 2024

joschmitt commented Jul 23, 2024

joschmitt commented Jul 23, 2024 •

edited

Loading

benlorenz commented Jul 23, 2024

benlorenz commented Jul 23, 2024

simonbrandhorst commented Jul 24, 2024 •

edited

Loading

		given as the formal sum of
		2 * component E8_3 of fiber over (0, 1)

Fix vinberg booktests #3939

Fix vinberg booktests #3939

Conversation

simonbrandhorst commented Jul 15, 2024 • edited by lgoettgens Loading

simonbrandhorst commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 15, 2024 • edited Loading

Codecov Report

simonbrandhorst commented Jul 15, 2024 • edited Loading

simonbrandhorst commented Jul 16, 2024

benlorenz commented Jul 16, 2024

HechtiDerLachs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrandhorst commented Jul 17, 2024

benlorenz commented Jul 17, 2024

simonbrandhorst commented Jul 17, 2024 • 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

thofma commented Jul 19, 2024

simonbrandhorst commented Jul 23, 2024

simonbrandhorst commented Jul 23, 2024

joschmitt commented Jul 23, 2024

joschmitt commented Jul 23, 2024 • edited Loading

benlorenz commented Jul 23, 2024

benlorenz commented Jul 23, 2024

simonbrandhorst commented Jul 24, 2024 • edited Loading

simonbrandhorst commented Jul 15, 2024 •

edited by lgoettgens

Loading

codecov bot commented Jul 15, 2024 •

edited

Loading

simonbrandhorst commented Jul 15, 2024 •

edited

Loading

simonbrandhorst commented Jul 17, 2024 •

edited

Loading

joschmitt commented Jul 23, 2024 •

edited

Loading

simonbrandhorst commented Jul 24, 2024 •

edited

Loading