Address various issues #50
Conversation
…racterToPrincipalDivisor
…visorsToClassGroup
…sInvariantDivisorGroup
…CotangentSheafViaEulerSequence
…riskiCotangentSheafViaPoincareResidueMap
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
==========================================
+ Coverage 98.68% 98.87% +0.18%
==========================================
Files 4 4
Lines 229 266 +37
==========================================
+ Hits 226 263 +37
Misses 3 3
Continue to review full report at Codecov.
|
dimemsion_of_torusfactor -> dim_of_torusfactor is_numerically_effective -> is_nef
HereAround
force-pushed
the
VariousIssues
branch
from
7efc094
to
5ee2132
Compare
| # Examples | ||
| ```julia-repl | ||
| julia> hirzebruch_surface( 5 ) | ||
| NormalToricVariety(GAP: <A toric variety of dimension 2>, Polymake.BigObjectAllocated(Ptr{Nothing} @0x0000562fbcc21b70)) |
There was a problem hiding this comment.
Perhaps install a nicer show method for NormalToricVariety
| """ | ||
| function del_pezzo( b::Int ) | ||
| if b < 0 | ||
| @warn("Number of blowups for construction of delPezzo surfaces must be non-negative.") |
There was a problem hiding this comment.
So why only a warning, not an error?
| end | ||
| if b > 3 | ||
| @warn("delPezzo surfaces with more than 3 blowups are realized as subvarieties of toric ambient spaces. This is currently not supported.") | ||
| return 0 |
There was a problem hiding this comment.
Again, why warn instead of error? This breaks type stability of this function
| @@ -61,6 +64,14 @@ using Test | |||
| @test ith_betti_number( H5, 4 ) == 1 | |||
| @test nr_of_q_rational_points( H5, 1 ) == 4 | |||
|
|
|||
| # Construct delPezzo surfaces | |||
| del_pezzo( -1 ) | |||
There was a problem hiding this comment.
There should be a way to test that this throws an error. In Oscar I saw something called @test_throws with a quick grep.
|
|
||
| Checks if the normal toric variety `v` is normal. (This function is somewhat tautological at this point.) | ||
|
|
||
| # Examples | ||
| ```julia-repl | ||
| julia> is_normal_variety( projective_space( 2 ) ) | ||
| julia> is_normal_variety(projective_space( 2 )) |
There was a problem hiding this comment.
So I don't mind how you do it, but isn't this inconsistent again? I'd either do this (GAP style)
| julia> is_normal_variety(projective_space( 2 )) | |
| julia> is_normal_variety( projective_space( 2 ) ) |
Or more likely (if I did it, I mean) in Julia style:
| julia> is_normal_variety(projective_space( 2 )) | |
| julia> is_normal_variety(projective_space(2)) |
| false | ||
| ``` | ||
| """ | ||
| function is_affine( v::normalToricVariety ) | ||
| function is_affine( v::NormalToricVariety ) | ||
| return v.polymakeNTV.AFFINE |
There was a problem hiding this comment.
Is this a always a boolean? Perhaps help the compiler via an annotation:
| return v.polymakeNTV.AFFINE | |
| return v.polymakeNTV.AFFINE::Bool |
| #end | ||
| #export underlying_sheaf | ||
|
|
||
|
|
||
| struct NefCone | ||
| polymakeNefCone::Polymake.BigObject |
There was a problem hiding this comment.
Why is this indented differently than the functions in this file?
| """ | ||
| mori_cone( v::NormalToricVariety ) | ||
|
|
||
| Computes the mori cone of the normal toric variety `v`. |
There was a problem hiding this comment.
Is it really mori cone, not a Mori cone?
|
|
||
| Compute the i-th Betti number of the normal toric variety `v`. | ||
| """ | ||
| function ith_betti_number( v::normalToricVariety, i::Int ) | ||
| function ith_betti_number( v::NormalToricVariety, i::Int ) | ||
| return GAP.Globals.ithBettiNumber( v.GapNTV, GapObj( i ) )::Int |
There was a problem hiding this comment.
Note that the GapObj is not necessary here, an Int can always be passed to a GAP function
|
@HereAround BTW, I am also willing to make some of the changes I suggest -- just let me know if you agree with the suggestion, and whether it is OK to change it, then I can push corresponding changes here. That may be more efficient. |
|
@fingolfin Sure.
|
|
There is a show method in my PR that comes after this. |
|
I'll merge this so that @lkastner PR can be rebased if needed, and will submit my tweaks in a separate PR. |
This PR aims to close the following issues: