Extended functionality and documentation #38

HereAround · 2021-09-05T16:15:17Z

Content of this PR:

Provide support for more GAP functions of the ToricVarieties_project, including Betti numbers.
Initiate documentation.

@lkastner @fingolfin : I am uncertain if I meet all Oscar standards with the documentation. I can produce the documentation locally (julia make.jl in the docs folder). What additional steps are needed to publish this documentation on github?

@lkastner Are you ok with the new wrapper functions?

@fingolfin I am uncertain what to make to the following Gap <-> Julia behaviour. On the one hand, execute th following in Julia:

using JToric
P2 = projective_space( 2 )
ith_betti_number( P2, 0 )
-2

The correct answer is 1, not -2.

The wrapper function ith_betti_number executes the corresponding gap function. If I directly execute this gap code, then I indeed obtain the correct answer 1 (after fixing a bug yesterday):

LoadPackage( "ToricVarieties" );
P2 := ProjectiveSpace( 2 );
ith_betti_number( P2, 0 );
1

So it looks as if the Gap return value 1 is turned into a -2 in Julia. Am I missing something? Suggestions?

[Aside: This is the reason why I am currently excluding the computation of the 0-th Betti numbers from the tests.]

codecov · 2021-09-05T16:42:51Z

Codecov Report

Merging #38 (736a41b) into master (35ad1fe) will decrease coverage by 0.80%.
The diff coverage is 97.32%.

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
- Coverage   96.00%   95.19%   -0.81%     
==========================================
  Files           4        4              
  Lines         200      229      +29     
==========================================
+ Hits          192      218      +26     
- Misses          8       11       +3

Impacted Files	Coverage Δ
src/JToric.jl	`72.72% <50.00%> (-27.28%)`	⬇️
src/AttributesAndMethods.jl	`100.00% <100.00%> (ø)`
src/ToricDivisors.jl	`100.00% <100.00%> (ø)`
src/ToricVarieties.jl	`85.45% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd84220...736a41b. Read the comment docs.

fingolfin · 2021-09-06T10:32:46Z

For publishing: you'll need a step doing that in your GitHub Actions. This is described in the Documenter.jl documentation. For a practical example, see here for how GAP.jl does it -- you may or may not want to remove the part where it runs the doctests.

fingolfin

Minor off-topic remarks

fingolfin · 2021-09-06T10:39:05Z

src/AttributesAndMethods.jl

 function map_from_weil_divisors_to_class_group( v::NormalToricVariety )
    gap_MapFromWeilDivisorsToClassGroup = GAP.Globals.MapFromWeilDivisorsToClassGroup( v.GapNTV )
    return map_from_weil_divisors_to_class_group( gap_MapFromWeilDivisorsToClassGroup )
 end
 export map_from_weil_divisors_to_class_group


+"""
+    dimension( v::NormalToricVariety )


everywhere else in Oscar, we are using dim right now.

Ok. I will change this to dim.

fingolfin · 2021-09-06T10:40:03Z

src/AttributesAndMethods.jl

+    name_of_variety( v::NormalToricVariety )
+
+Returns the name of the normal toric variety `v`, if set. Otherwise returns "No name set for this variety".
+"""
 function name_of_variety( v::NormalToricVariety )
    if ! ( Bool( GAP.Globals.HasNameOfVariety( v.GapNTV ) ) )


There is no need for Bool here, you can just write if ! GAP.Globals.HasNameOfVariety(v.GapNTV)

Thank you for the pointer. I will change this.

fingolfin · 2021-09-06T10:44:54Z

I'll look into the ith_betti_number weirdness.

fingolfin · 2021-09-06T21:40:13Z

@HereAround I could not reproduce the "-2" at first because I get this error instead:

julia> ith_betti_number(P2, 0)
#I  The Julia package 'Polymake' cannot be loaded.
ERROR: Error thrown by GAP: Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 2nd choice method found for `FacetInequalities' on 1 arguments at /Users/mhorn/.julia/artifacts/8f22bf2645df36936a8a74cea9de55d6070017ed/share/gap/lib/methsel2.g:249 called from
FacetInequalities( polytope ) at /Users/mhorn/.julia/gaproot/v4.12/pkg/NConvex/gap/Polytope.gi:789 called from
NormalFan( PolytopeOfVariety( variety ) ) at /Users/mhorn/.julia/packages/CapAndHomalg/PE2rB/pkg/ToricVarieties_project/ToricVarieties/gap/ProjectiveToricVarieties.gi:88 called from
FanOfVariety( variety ) at /Users/mhorn/.julia/packages/CapAndHomalg/PE2rB/pkg/ToricVarieties_project/ToricVarieties/gap/ToricVarieties.gi:1026 called from
<function "ithBettiNumber for toric varieties">( <arguments> )
 called from read-eval loop at *defin*:0

Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] ThrowObserver(depth::Int32)
   @ GAP ~/.julia/packages/GAP/9XZl9/src/GAP.jl:63
 [3] _call_gap_func(func::GAP.GapObj, a1::GAP.GapObj, a2::Int64)
   @ GAP ~/.julia/packages/GAP/9XZl9/src/ccalls.jl:268
 [4] call_gap_func_nokw
   @ ~/.julia/packages/GAP/9XZl9/src/ccalls.jl:222 [inlined]
 [5] (::GAP.GapObj)(a1::GAP.GapObj, a2::Int64)
   @ GAP ~/.julia/packages/GAP/9XZl9/src/ccalls.jl:233
 [6] ith_betti_number(v::NormalToricVariety, i::Int64)
   @ JToric ~/Projekte/OSCAR/JToric/src/AttributesAndMethods.jl:583
 [7] top-level scope
   @ REPL[12]:1

I think this is caused by JConvex doing JuliaImportPackage("Polymake") -- but in the Julia project I have, there is only one top-level package (JToric), so this doesn't work, even though I have Polymake.jl installed.

This could be addressed in various ways; e.g. JToric could, in its __init__ function, do something like GAP.Globals.__Polymake_jl = Polymake and then JToric could look at the Julia module object... see also oscar-system/GAP.jl#687

fingolfin · 2021-09-06T21:43:35Z

Indeed, with Polymake.jl loaded, I can also reproduce this from the GAP prompt, so I would suggest taking a look at the code in JConvex:

julia> JToric.GAP.prompt()
gap> LoadPackage( "ToricVarieties" ); # of course it was already loaded by JToric
true
gap> P2 := ProjectiveSpace( 2 );
<A projective toric variety of dimension 2>
gap> ithBettiNumber(P2, 0);
-2

fingolfin · 2021-09-06T21:46:48Z

Just to illustrate how I think one can avoid using JuliaImportPackage, consider this (executed in Julia session after I did using JToric, Polymake)

julia> Polymake.version()
v"4.4.0"

julia> JToric.GAP.Globals.pm = Polymake
Polymake

julia> JToric.GAP.prompt()
gap> pm.version();
<Julia: v"4.4.0">

HereAround · 2021-09-06T22:09:00Z

Ok. It seems we have multiple issues here.

Properly call/import Polymake in JToric. If I understand correctly, then you propose to extend the __init__ of JToric by JToric.GAP.Globals.pm = Polymake and then making the calls to Polymake via pm.prefered_function? I understand you correctly?
The ith_betti_number. Good point! Indeed

julia> using JToric;
julia> P2 = projective_space( 2 );
julia> GAP.Globals.FVector( GAP.Globals.FanOfVariety( P2.GapNTV ) )
GAP: [ 3, 0 ]

This should be the number of 1 and 2-dimensional cones, i.e. should be [3,3]. I will investigate.

fingolfin · 2021-09-09T08:47:17Z

Yes regarding point 1. -- except that pm was just an example, it might be better to call it something like __Polymake or __PolymakeFromJulia or so or POLYMAKE_JULIA_MODULE or whatever, just to avoid clashes with user objects.

fingolfin · 2021-09-09T08:56:57Z

Another thing: I noticed that in JConvex, you use JuliaEvalString a lot. That's inefficient and should almost never be necessary. It also leads to hard to debug code.

For example, if one defined pm as above, then this works just fine in GAP: pm.polytope.Polytope()

However, it also seems as if your function Polymake_V_Rep_command_string constantly creates new Polymake objects? Is there a reason why you don't just create one of these once, and keep it around? Or am I misunderstanding the code?

lkastner · 2021-09-13T13:36:41Z

src/ToricDivisors.jl

@@ -90,6 +150,11 @@ end
 export is_very_ample


+"""
+    is_numerically_effective( d::toric_divisor )


Is it not common to abbreviate this to nef these days?

Thx. Just changed it.

lkastner · 2021-09-13T13:39:07Z

src/AttributesAndMethods.jl


+"""
+    dimension_of_torusfactor( v::NormalToricVariety )


Should we change this to dim_of_torusfactor analogously then?

Yes, that is a good point. Changed as well.

HereAround · 2021-09-17T02:28:19Z

@fingolfin :

Thank you for suggesting these improvements! To handle them efficiently, I have opened the following issues:

I will try to close them soonish.

…ivisorGroup

…omCharacterToPrincipalDivisor

…lDivisorsToClassGroup

…TorusInvariantDivisorGroup

…riskiCotangentSheafViaEulerSequence

… -> zariskiCotangentSheafViaPoincareResidueMap

…ionalFunction

This reverts commit 786d85f.

HereAround · 2021-09-22T15:46:05Z

@fingolfin I have reformatted all structs into camelCase. Also, I reverted the commit regarding GAP.Globals.POLYMAKE_JULIA_MODULE that you commented on above.

With these changes - are you ready to approve this PR?

fingolfin · 2021-09-23T21:21:43Z

src/ToricDivisors.jl

 ######################
 # 1: The Julia type for ToricDivisors
 ######################

-struct toric_divisor
+struct toricDivisor


So, I am realllly sorry for all the churn and work I cause you... but really the convention in Julia is to use CamelCase for structs, not camelCase...

Oh... This one is on me. You wrote CamelCase above - I should have read carefully. Hence, no worries!

Since this PR is already terribly long (and you just approved), I opened a new issue: #44.

This one should be closed with the adjustment to CamelCase soon.

src/ToricVarieties.jl

fingolfin · 2021-09-23T21:26:42Z

src/ToricVarieties.jl

+Checks if the normal toric variety `v` is projective, i.e. if the fan of `v` is the the normal fan of a polytope.
+
+# Examples
+```julia-repl


You could also use jldoctest, then these examples would also be run as part of the doctest step of the CI (provided that is set up), which has the advantage that the examples are tested, so one knows they actually are working and correct. But of course sometimes this is not desirable, e.g. if outputs are random or differ between Julia versions. Seems to be safe here, though?

Anyway, not important right now, but perhaps you'll want to change this down the road, so I thought I should mention it.

Thank you for the suggestion. Sounds like a valuable addition!

For the time being (and in repeating the above strategy), I placed this in a separated issue: #46.

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

HereAround requested review from fingolfin and lkastner September 5, 2021 16:55

fingolfin reviewed Sep 6, 2021

View reviewed changes

lkastner approved these changes Sep 13, 2021

View reviewed changes

HereAround force-pushed the ExtendedFunctionalityAndDocumentation branch from ab4e741 to c710af8 Compare September 17, 2021 02:14

This was referenced Sep 17, 2021

Improve calling of Polymake #40

Open

Improvement on Polymake_V_Rep_command_string (and similar for H_Rep) homalg-project/ToricVarieties_project#141

Closed

HereAround mentioned this pull request Sep 17, 2021

Remove JuliaEvalString homalg-project/ToricVarieties_project#140

Closed

lkastner approved these changes Sep 17, 2021

View reviewed changes

HereAround added 11 commits September 18, 2021 12:06

Add support for GAP function CharacterToRationalFunction

b1bd5f0

Add support for GAP function WeilDivisorsOfVariety

0082f6c

Add support for Gap function Factors

a2c490a

Add support for GAP function BlowUpOnIthMinimalTorusOrbit

e67ab59

Add support for GAP function ZariskiCotangentSheafViaEulerSequence

5783652

Add support for GAP function ZariskiCotangentSheafViaPoincareResidueMap

3665738

Add support for Gap function ith_betti_number

1c35bc9

Add support for Gap function NrOfqRationalPoints

fbea93b

Restructure the tests

0cde5d9

Fix typo in documentation of function JToric.version

c8bf726

Restructure include statements

229c420

HereAround added 20 commits September 22, 2021 08:42

camelCase for struct Cox_ring -> coxRing

75a56aa

camelCase for struct cox_ring -> coxRing

99e5ccc

camelCase for struct class_group -> classGroup

23d79c8

camelCase for struct torus_invariant_divisor_group -> torusInvariantD…

2a04668

…ivisorGroup

camelCase for struct map_from_character_to_principal_divisor -> mapFr…

929432c

…omCharacterToPrincipalDivisor

camelCase for struct map_from_weil_divisors_to_class_group mapFromWei…

08c56fb

…lDivisorsToClassGroup

camelCase for struct coordinate_ring_of_torus -> coordinateRingOfTorus

6557efb

camelCase for struct character_lattice -> characterLattice

9ce5c35

camelCase for struct irrelevant_ideal -> irrelevantIdeal

b7d58a2

camelCase for struct stanley_reisner_ideal -> stanleyReisnerIdeal

1fcd396

camelCase for struct morphism_from_cox_variety -> morphismFromCoxVariety

5f14acd

camelCase for struct cartier_torus_invariant_divisor_group -> cartier…

2041773

…TorusInvariantDivisorGroup

camelCase for struct picard_group -> picardGroup

9d5b049

camelCase for struct zariski_cotangent_sheaf -> zariskiCotangentSheaf

3489613

camelCase for struct cotangent_sheaf -> cotangentSheaf

c1828e8

camelCase for struct zariski_cotangent_sheaf_via_euler_sequence -> za…

b54f5c0

…riskiCotangentSheafViaEulerSequence

camelCase for struct zariski_cotangent_sheaf_via_poincare_residue_map…

696d6b4

… -> zariskiCotangentSheafViaPoincareResidueMap

camelCase for struc underlying_sheaf -> underlyingSheaf

c29a409

camelCase for struct character_to_rational_function -> characterToRat…

3dc6648

…ionalFunction

Revert "Define JToric.GAP.Globals.POLYMAKE_JULIA_MODULE"

917c4fa

This reverts commit 786d85f.

fingolfin reviewed Sep 23, 2021

View reviewed changes

fingolfin approved these changes Sep 23, 2021

View reviewed changes

This was referenced Sep 23, 2021

All structs must use CamelCase #44

Closed

Remove extra white spaces from examples/tests. #45

Closed

Remove extra white space

736a41b

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

HereAround mentioned this pull request Sep 23, 2021

Use jldoctest to test examples #46

Closed

HereAround merged commit 4de97a8 into oscar-system:master Sep 23, 2021

HereAround deleted the ExtendedFunctionalityAndDocumentation branch September 23, 2021 22:02

HereAround mentioned this pull request Sep 26, 2021

Activate the doctests #52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended functionality and documentation #38

Extended functionality and documentation #38

HereAround commented Sep 5, 2021 •

edited

Loading

codecov bot commented Sep 5, 2021 •

edited

Loading

fingolfin commented Sep 6, 2021

fingolfin left a comment

fingolfin Sep 6, 2021

HereAround Sep 6, 2021

fingolfin Sep 6, 2021

HereAround Sep 6, 2021

fingolfin commented Sep 6, 2021

fingolfin commented Sep 6, 2021

fingolfin commented Sep 6, 2021

fingolfin commented Sep 6, 2021

HereAround commented Sep 6, 2021

fingolfin commented Sep 9, 2021

fingolfin commented Sep 9, 2021

lkastner Sep 13, 2021

HereAround Sep 17, 2021

lkastner Sep 13, 2021

HereAround Sep 17, 2021

HereAround commented Sep 17, 2021

HereAround commented Sep 22, 2021

fingolfin Sep 23, 2021

HereAround Sep 23, 2021

fingolfin Sep 23, 2021

HereAround Sep 23, 2021

Extended functionality and documentation #38

Extended functionality and documentation #38

Conversation

HereAround commented Sep 5, 2021 • edited Loading

codecov bot commented Sep 5, 2021 • edited Loading

Codecov Report

fingolfin commented Sep 6, 2021

fingolfin left a 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

fingolfin commented Sep 6, 2021

fingolfin commented Sep 6, 2021

fingolfin commented Sep 6, 2021

fingolfin commented Sep 6, 2021

HereAround commented Sep 6, 2021

fingolfin commented Sep 9, 2021

fingolfin commented Sep 9, 2021

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

HereAround commented Sep 17, 2021

HereAround commented Sep 22, 2021

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

HereAround commented Sep 5, 2021 •

edited

Loading

codecov bot commented Sep 5, 2021 •

edited

Loading