Elliptic surfaces fixes #4177
Elliptic surfaces fixes #4177
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4177 +/- ##
==========================================
- Coverage 84.59% 84.58% -0.01%
==========================================
Files 631 631
Lines 84813 84826 +13
==========================================
+ Hits 71747 71754 +7
- Misses 13066 13072 +6
|
| V_ref = [(U, (UV, h)) for (U, (UV, h)) in decomp_dict if UV === V] | ||
| _compl(a) = sum(length(lifted_numerator(b)) + length(lifted_denominator(b)) for b in a[2][2]; init=0) |
There was a problem hiding this comment.
A minor and purely optional nitpick (feel free to ignore):
So UV is always V. So why not do this?
| V_ref = [(U, (UV, h)) for (U, (UV, h)) in decomp_dict if UV === V] | |
| _compl(a) = sum(length(lifted_numerator(b)) + length(lifted_denominator(b)) for b in a[2][2]; init=0) | |
| V_ref = [(U, h) for (U, (UV, h)) in decomp_dict if UV === V] | |
| _compl(a) = sum(length(lifted_numerator(b)) + length(lifted_denominator(b)) for b in a[2]; init=0) |
and then the for loop in line 115 can be slightly simplified, and also line 120.
This avoids a little bit of storage space for those tuples.
Anyway, perhaps you have a reason for having it as it is (e.g. plans for further changes, keeping the code similar to something else etc.) -- so as I said, feel free to ignore.
There was a problem hiding this comment.
This makes sense. I just cluttered it together so that it would work for the moment. Thanks for the close look.
| @@ -1649,6 +1649,19 @@ function extended_ade(ADE::Symbol, n::Int) | |||
| return -G, kernel(G; side = :left) | |||
| end | |||
|
|
|||
| # This function allows to store a reduction map to positive characteristic, | |||
| # e.g. for computing intersection numbers. | |||
| function reduction_to_pos_char(X::EllipticSurface, red_map::Map) | |||
There was a problem hiding this comment.
Should we turn this into a field of EllipticSurface?
There was a problem hiding this comment.
Let's discuss this in #4183 .
| @@ -784,19 +784,6 @@ end | |||
| return C | |||
| end | |||
|
|
|||
| function inherit_decomposition_info!(C::Covering, X::AbsCoveredScheme) | |||
There was a problem hiding this comment.
why remove this? Was it a duplicate?
There was a problem hiding this comment.
Yes. And it was wrong. Fortunately, it seemed to be used nowhere.
| @@ -345,7 +345,6 @@ end | |||
| end | |||
|
|
|||
| @attr Bool function has_dimension_leq_zero(I::Ideal) | |||
| is_one(I) && return true | |||
There was a problem hiding this comment.
Why remove this line?
There was a problem hiding this comment.
It's superfluous. It's usually not easier to compute this than the dimension. Both are the same Groebner basis computation.
There was a problem hiding this comment.
Yep. Is the GB stored? Or could it be that it is discarded and just the is_one attribute is true?
inherit_decomposition_info!has_dim_leq_zero