fixed bug in membership test_function

[mohamed-barakat]

the assumption made on HilbertPolynomial in the code is wrong

I understood from @HereAround that he has transferred some of the functionality of ToricSheaves. In this case, these fixes should also be relevant to his code.

[codecov[bot]]

Codecov Report

Merging #636 (b7357f2) into master (46c2298) will decrease coverage by 8.06%. The diff coverage is 100.00%.

:exclamation: Current head b7357f2 differs from pull request most recent head a168c0a. Consider uploading reports for the commit a168c0a to get more accurate results

@@            Coverage Diff             @@
##           master     #636      +/-   ##
==========================================
- Coverage   70.78%   62.72%   -8.07%     
==========================================
  Files         328      326       -2     
  Lines       47456    44277    -3179     
==========================================
- Hits        33594    27773    -5821     
- Misses      13862    16504    +2642     

Impacted Files	Coverage Δ
ToricSheaves/PackageInfo.g	98.11% <100.00%> (ø)
...iteOfCategoryOfRowsOfCommutativeRingPrecompiled.gi	0.52% <0.00%> (-89.76%)	:arrow_down:
...goryOfGradedRowsAndColumns/CategoryOfGradedRows.gi	1.22% <0.00%> (-88.91%)	:arrow_down:
...yOfGradedRowsAndColumns/CategoryOfGradedColumns.gi	1.22% <0.00%> (-88.66%)	:arrow_down:
FreydCategoriesForCAP/gap/CokernelImageClosure.gi	8.55% <0.00%> (-83.13%)	:arrow_down:
FreydCategoriesForCAP/gap/RingsAsAbCats.gi	16.98% <0.00%> (-81.75%)	:arrow_down:
FreydCategoriesForCAP/gap/FreydCategory.gi	6.97% <0.00%> (-80.26%)	:arrow_down:
FreydCategoriesForCAP/gap/GradeFiltration.gi	28.57% <0.00%> (-71.43%)	:arrow_down:
FreydCategoriesForCAP/gap/Relations.gi	20.66% <0.00%> (-69.43%)	:arrow_down:
AttributeCategoryForCAP/gap/AttributeCategory.gi	14.14% <0.00%> (-67.23%)	:arrow_down:
... and 110 more

---

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 46c2298...a168c0a. Read the comment docs.

[HereAround]

the assumption made on HilbertPolynomial in the code is wrong

I understood from @HereAround that he has transferred some of the functionality of ToricSheaves. In this case, these fixes should also be relevant to his code.

@mohamed-barakat Thank you for the pointer. Your critical change is, I think, already fixed in https://github.com/homalg-project/ToricVarieties_project/blob/7dc7c5af027e4d9e52b3032ac7d77cb97a7e8a2a/CoherentSheavesOnToricVarieties/gap/CoherentSheaves.gi#L29.

Note that this line is identical to your change, except that degree_matrix_as_list_list has been renamed into degrees. Also comes_from_smooth_variety is identical to testing if the underlying variety is smooth.

[sebastianpos]

Just to be sure, are you confident that the Hilbert polynomial works even under the new hypotheses?

The following example makes me doubt it, but maybe I get something wrong: If you remove from the projective space the hyperplane x_0 = 0, you get a smooth toric variety with the same Cox ring as the projective space, but enlarged irrelevant locus. The Hilbert polynomial should not be able to detect that.

[mohamed-barakat]

Just to be sure, are you confident that the Hilbert polynomial works even under the new hypotheses?

One also needs that the irrelevant locus is zero-dimensional. The Hilbert polynomial invoked by the code is the classical Hilbertpolynomiall assuming all degrees equal to 1.

The following example makes me doubt it, but maybe I get something wrong: If you remove from the projective space the hyperplane x_0 = 0, you get a smooth toric variety with the same Cox ring as the projective space, but enlarged irrelevant locus. The Hilbert polynomial should not be able to detect that.

I probably misunderstand your example. If I delete x_0 = 0 I get the affine space with one ray-generator less (so different Cox ring) and trivial degree group. What is the fan in your example?

[sebastianpos]

One also needs that the irrelevant locus is zero-dimensional. The Hilbert polynomial invoked by the code is the classical Hilbertpolynomiall assuming all degrees equal to 1.

Does this comment imply that the new hypotheses are still insufficient?

What other cases beside projective spaces can be covered by the Hilbert polynomial? If there are none, then maybe the if clause should be replaced by checking whether we simply have a projective space?

I'm not familiar with the intended framework of this package. Does every modelled toric variety needs to come from a fan? Or is it sufficient to supply a Cox ring together with an irrelevant ideal? The second framework is more general, since it allows my given counter-example (the affine space modelled in an usual way).

[mohamed-barakat]

Does this comment imply that the new hypotheses are still insufficient?

Yes.

What other cases beside projective spaces can be covered by the Hilbert polynomial? If there are none, then maybe the if clause should be replaced by checking whether we simply have a projective space?

For example the blowup of A^2 at the origin. The resulting normal toric variety is smooth with class group Z and irrelevant locus <x,y>.

I'm not familiar with the intended framework of this package. Does every modelled toric variety needs to come from a fan? Or is it sufficient to supply a Cox ring together with an irrelevant ideal? The second framework is more general, since it allows my given counter-example (the affine space modelled in an usual way).

Since this package is based on ToricVarieties I assumed it requires normality.

[sebastianpos]

Does this comment imply that the new hypotheses are still insufficient?

Yes.

Do you want this PR to be merged anyway? Or do you intend to correct it?

For example the blowup of A^2 at the origin. The resulting normal toric variety is smooth with class group Z and irrelevant locus <x,y>.

The Cox ring in this example is k[t,x,y] with deg(t) = -1, deg(x) = deg(y) = 1. The current if-clause excludes this example since one of the degrees is negative, if I'm not mistaken. My question remains: can this if-clause simply be reduced to checking whether we have a projective space?

[mohamed-barakat]

You are right, the easiest is to restrict to the projective space and generalize at a later point with an appropriate notion of a multigraded Hilbert polynomial. I will adapt the PR.

[mohamed-barakat]

For the record:

• The input of method for CategoryOfToricSheaves is (graded_ring, irrelevant_ideal_generators, and a boolean comes_from_smooth_variety).
• This method is called from another method for the same operation which takes a normal toric variety as input. This is the only way how the above method is invoked in the examples.
• The package ToricVarieties has a method IsIsomorphicToProjectiveSpace which additionally checks IsProjective( variety ) and otherwise differs from the condition altered in this PR by requiring that all weights are = 1.