ToricVarieties: Several bugfixes #1909

HereAround · 2023-02-08T14:52:31Z

Frederik Witt reported the following bug to me:

|julia> H2=hirzebruch_surface(2);

|julia> transpose(matrix(ZZ, rays(H2)))
[1   0   -1    0]
[0   1    2   -1]

|julia> matrix(map_from_character_lattice_to_torusinvariant_weil_divisor_group(H2))
[0   1    2   -1]
[1   0   -1    0]

The two matrices should be identical, but apparently are not. The rows are exchanged.

In the constructor for the Hirzebruch surface, an attribute was set incorrectly. This is the root cause for this behavior. This PR fixes this in the constructor for the projective space, Hirzebruch and del Pezzo surfaces.

While investigating this issue, I also noticed a similar glitch. For a smooth toric variety, we proceed as follows:

Set map_from_torusinvariant_weil_divisor_group_to_class_group.
Set map_from_cartier_divisor_group_to_picard_group as being identical to map_from_torusinvariant_weil_divisor_group_to_class_group.

It then follows, that Picard group and Class group of the smooth variety in question are identical (as in are the exact same object in the memory of the computer). To me this seems odd, even though Picard and Class group are isomorphic in such a setting. So I have modified the code such that they are isomorphic but not identical.

Another bug that Frederik Witt reported was the following:

|julia> P2 = projective_space(NormalToricVariety, 2);

|julia> td = ToricDivisor(P2,[1,-1,0]);

|julia> is_effective(td)
true

A toric Weil divisor is effective iff all of its coefficients are non-negative. So td is not effective, in contrast to the output by OSCAR. I have done the following:

I modified the existing is_effective function of a toric divisor: It now checks if all coefficients are non-negative. If so, it returns true and otherwise false.
I have added a new function is_linearly_equivalent_to_effective_toric_divsior: This constructs the corresponding toric divisor in Polymake and then checks if this divisor is linearly equivalent to an effective toric divisor. (This is the behavior of is_effective before this PR.)
I have added a new function is_effective for toric divisor classes: It finds a toric divisor in the divisor class at hand and then checks if this divisor is linearly equivalent to an effective toric divisor.

Tests and documentation for the new functions have been added. Those for the old functions have been modified.

fingolfin

Looks good to me, a few very minor suggestions

src/ToricVarieties/ToricDivisorClasses/properties.jl

fingolfin · 2023-02-10T09:06:01Z

src/ToricVarieties/ToricDivisors/properties.jl

 true
 ```
 """
-@attr Bool is_effective(td::ToricDivisor) = pm_tdivisor(td).EFFECTIVE
+@attr Bool is_effective(td::ToricDivisor) = all(c -> (c >= 0), coefficients(td))


So what did the old code compute then?!

It checked whether the toric divisor td is linearly equivalent to an effective divisor. In other words, it checked whether there exists a principal divisor p s.t. td + p is effective.

test/ToricVarieties/divisor_classes.jl

Standard constructor must not PERMUTE rays to form map from character lattice to Weil divisors (Co)Domains of maps set by default constructors must match the corresponding groups of the variety

It checks if a divisor is lin. equivalent to an effective divisor. But it should check if the divisor is effective.

Co-authored-by: Max Horn <max@quendi.de>

HereAround · 2023-02-10T12:33:37Z

The error in the required tests / test (~1.8.0-0, x64, ubuntu-latest) (pull_request) stems (if I read the log correctly) from the following:

Examples.factor_absolute: Error During Test at /home/runner/work/Oscar.jl/Oscar.jl/test/Experimental/ModStdQt-test.jl:17

For tests / test (~1.9.0-0, x64, ubuntu-latest) (pull_request), the failure seems to stem from

Experimental LocalH2: Error During Test at /home/runner/work/Oscar.jl/Oscar.jl/test/Experimental/gmodule-test.jl:30

Both files have not been touched in this PR and all tests (except for the time-out on macOS - cf. #1888) succeeded yesterday. I am not sure why I am seeing this error.

Any suggestions @thofma and @fieker? Is this possibly related to #1912?

benlorenz · 2023-02-10T12:43:33Z

Examples.factor_absolute: Error During Test at ...

The factor_absolute failure comes from this PR that was merged despite this failing (but it doesn't always fail).

Experimental LocalH2: Error During Test at ...
Any suggestions @thofma and @fieker? Is this possibly related to #1912?

The Local H2 error looks very similar to the one I reported in #1912, yes.

Both errors will probably go away when re-running the tests.

HereAround · 2023-02-10T13:03:25Z

Examples.factor_absolute: Error During Test at ...

The factor_absolute failure comes from this PR that was merged despite this failing (but it doesn't always fail).

Experimental LocalH2: Error During Test at ...
Any suggestions @thofma and @fieker? Is this possibly related to #1912?

The Local H2 error looks very similar to the one I reported in #1912, yes.

Both errors will probably go away when re-running the tests.

Thank you for elaborating. Let me rerun the tests...

benlorenz · 2023-02-10T13:13:52Z

Let me rerun the tests...

A quick tip for the next time: Instead of re-opening the ticket you can also go to the Checks tab, select the Run tests job on the left, then on the top-right Cancel (if e.g. nightly is still running) and finally Re-run just the failed jobs. This way you can just avoid running all of them again.
In that view you can also re-run single workflows by hovering over the job and clicking that refresh icon that will appear, see here. (This works only once all jobs have finished)

PS: I have re-run way too many jobs to debug that nightly issue...

HereAround · 2023-02-10T13:20:37Z

Let me rerun the tests...

A quick tip for the next time: Instead of re-opening the ticket you can also go to the Checks tab, select the Run tests job on the left, then on the top-right Cancel (if e.g. nightly is still running) and finally Re-run just the failed jobs. This way you can just avoid running all of them again. In that view you can also re-run single workflows by hovering over the job and clicking that refresh icon that will appear, see here. (This works only once all jobs have finished)

PS: I have re-run way too many jobs to debug that nightly issue...

Thank you for the pointer. I shall remember that now. Indeed, this is the better option. (I actually recalled that option, but could not find the corresponding button a few minutes ago... I must have overlooked it. Thank you for telling me where to look.)

HereAround · 2023-02-13T10:01:08Z

Indeed, after rerunning the tests, all checks pass (other than the time-out for the macOS test). So this is ready for review @lkastner and @fingolfin . Thank you!

lkastner

Presetting all those properties for e.g. Hirzebruch surfaces always makes me a bit nervous, since it essentially makes it useless to have tests on these. I am a bit afraid that at some point we will run into situations where we have a bug, but it won't manifest for such standard constructions, even though it should, since we are assigning so many properties manually. Is it just me? Is there some kind of OSCAR policy regarding this?

Aside from that, great work.

lkastner · 2023-02-14T09:17:05Z

src/ToricVarieties/ToricDivisors/properties.jl

+@doc Markdown.doc"""
+    is_linearly_equivalent_to_effective_toric_divisor(td::ToricDivisor)
+
+Determine whether the toric divisor `td` is linearly equivalent to an effective toric divisor.
+
+# Examples
+```jldoctest
+julia> P2 = projective_space(NormalToricVariety,2)
+A normal, non-affine, smooth, projective, gorenstein, fano, 2-dimensional toric variety without torusfactor
+
+julia> td = ToricDivisor(P2, [1,-1,0])
+A torus-invariant, non-prime divisor on a normal toric variety
+
+julia> is_effective(td)
+false
+
+julia> is_linearly_equivalent_to_effective_toric_divisor(td)
+true
+```
+"""
+@attr Bool is_linearly_equivalent_to_effective_toric_divisor(td::ToricDivisor) = pm_tdivisor(td).EFFECTIVE
+export is_linearly_equivalent_to_effective_toric_divisor


Do we really want this? Can't one just use the class construction? I mean we could end up with tons of is_linearly_equivalent_to_* functions. Is this something we want to avoid or would it be ok?

I thought this might be convenient, but on second thought you might be right. As I see it (now), we could remove this.

HereAround · 2023-02-14T10:04:55Z

Presetting all those properties for e.g. Hirzebruch surfaces always makes me a bit nervous, since it essentially makes it useless to have tests on these. I am a bit afraid that at some point we will run into situations where we have a bug, but it won't manifest for such standard constructions, even though it should, since we are assigning so many properties manually. Is it just me? Is there some kind of OSCAR policy regarding this?

Aside from that, great work.

I agree that this could happen. The first bug above was sort-of similar: Issue for the default/standard constructor hirzebruch_surface() and no issue if you constructed it by hand from a fan.

About feeling uneasy: Personally, I am not too worried. Originally, the intention was to preset properties for speed-up. But then, my feeling is that we do not gain too much from this (but I have not done any benchmarking) and so would be happy to remove them.

HereAround marked this pull request as draft February 8, 2023 14:52

HereAround added bug Something isn't working topic: toric varieties labels Feb 8, 2023

HereAround force-pushed the Bugfix2 branch from 57c8e9c to 4639f19 Compare February 8, 2023 15:14

HereAround changed the title ~~ToricVarieties: Bugfixes~~ ToricVarieties: Several bugfixes Feb 8, 2023

HereAround force-pushed the Bugfix2 branch 2 times, most recently from f3b2536 to 9bbc6ab Compare February 8, 2023 18:54

HereAround requested a review from lkastner February 8, 2023 20:46

HereAround marked this pull request as ready for review February 8, 2023 20:46

HereAround force-pushed the Bugfix2 branch from 9bbc6ab to 6e81e5e Compare February 8, 2023 21:20

fingolfin reviewed Feb 10, 2023

View reviewed changes

HereAround and others added 3 commits February 10, 2023 11:46

ToricVarieties bugfix

f82ae5a

Standard constructor must not PERMUTE rays to form map from character lattice to Weil divisors (Co)Domains of maps set by default constructors must match the corresponding groups of the variety

ToricVarieties bugfix: is_effective(ToricDivisor)

4edaad4

It checks if a divisor is lin. equivalent to an effective divisor. But it should check if the divisor is effective.

ToricVarieties: Code optimization

40146ee

Co-authored-by: Max Horn <max@quendi.de>

HereAround force-pushed the Bugfix2 branch from 6d92d06 to 40146ee Compare February 10, 2023 10:46

HereAround closed this Feb 10, 2023

HereAround reopened this Feb 10, 2023

HereAround requested a review from fingolfin February 13, 2023 10:01

HereAround mentioned this pull request Feb 13, 2023

Reduce runtimes for tests #1929

Open

lkastner approved these changes Feb 14, 2023

View reviewed changes

HereAround merged commit 34e36c4 into oscar-system:master Feb 14, 2023

HereAround deleted the Bugfix2 branch February 14, 2023 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ToricVarieties: Several bugfixes #1909

ToricVarieties: Several bugfixes #1909

HereAround commented Feb 8, 2023 •

edited

Loading

fingolfin left a comment

fingolfin Feb 10, 2023

HereAround Feb 10, 2023 •

edited

Loading

HereAround commented Feb 10, 2023

benlorenz commented Feb 10, 2023 •

edited

Loading

HereAround commented Feb 10, 2023

benlorenz commented Feb 10, 2023

HereAround commented Feb 10, 2023

HereAround commented Feb 13, 2023

lkastner left a comment •

edited

Loading

lkastner Feb 14, 2023

HereAround Feb 14, 2023

HereAround commented Feb 14, 2023

ToricVarieties: Several bugfixes #1909

ToricVarieties: Several bugfixes #1909

Conversation

HereAround commented Feb 8, 2023 • edited Loading

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Feb 10, 2023

Choose a reason for hiding this comment

HereAround Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

HereAround commented Feb 10, 2023

benlorenz commented Feb 10, 2023 • edited Loading

HereAround commented Feb 10, 2023

benlorenz commented Feb 10, 2023

HereAround commented Feb 10, 2023

HereAround commented Feb 13, 2023

lkastner left a comment • edited Loading

Choose a reason for hiding this comment

lkastner Feb 14, 2023

Choose a reason for hiding this comment

HereAround Feb 14, 2023

Choose a reason for hiding this comment

HereAround commented Feb 14, 2023

HereAround commented Feb 8, 2023 •

edited

Loading

HereAround Feb 10, 2023 •

edited

Loading

benlorenz commented Feb 10, 2023 •

edited

Loading

lkastner left a comment •

edited

Loading