sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.43k stars 478 forks source link

Fix normalization in cohomology ring of orbifold toric varieties #12361

Closed vbraun closed 12 years ago

vbraun commented 12 years ago

For simplicial toric varieties, the rational cohomology ring and the ratironal Chow group are isomorphic. So if all normalizations are correct, then one should be able to do intersection computations in the cohomology ring. This patch fixes the volume_class() and the constructor of cohomology cycles from cones to make everything work.

It turns out that index_in_saturation does not work for trivial zero-size matrices, this is also fixed.

Apply attachment: trac_12361_fix_toric_cohomology_ring.patch, attachment: trac_12361_index_in_saturation_fix.patch

CC: @novoselt @sagetrac-davideklund

Component: algebraic geometry

Author: Volker Braun

Reviewer: David Eklund, Andrey Novoseltsev

Merged: sage-5.1.beta0

Issue created by migration from https://trac.sagemath.org/ticket/12361

vbraun commented 12 years ago

Attachment: trac_12361_index_in_saturation_fix.patch.gz

Initial patch

vbraun commented 12 years ago

Description changed:

--- 
+++ 
@@ -1 +1,5 @@
 For simplicial toric varieties, the rational cohomology ring and the ratironal Chow group are isomorphic. So if all normalizations are correct, then one should be able to do intersection computations in the cohomology ring. This patch fixes the `volume_class()` and the constructor of cohomology cycles from cones to make everything work.
+
+It turns out that index_in_saturation does not work for trivial zero-size matrices, this is also fixed.
+
+Apply  trac_12361_fix_toric_cohomology_ring.patch, trac_12361_index_in_saturation_fix.patch
novoselt commented 12 years ago

Reviewer: Andrey Novoseltsev

novoselt commented 12 years ago
comment:2

The patchbot complains about added whitespace! Looking at the actual code...

novoselt commented 12 years ago
comment:3

That's a really great expansion of the documentation!

I think I caught some typos:

novoselt commented 12 years ago

Work Issues: whitespaces and typos

novoselt commented 12 years ago
comment:4

OK, other than the above looks good!

1d88307d-4b29-4822-834f-5ebef9986059 commented 12 years ago
comment:5

Hi. I looked at this and I agree that it looks good.

One thing:

On line 215 of toric_variety.py it says "For toric varieties with at most orbifold singularities, the rational cohomology ring H(X,\QQ) and the rational Chow ring H(X,\QQ) are isomorphic."

It's a bit confusing that they are both denoted by H(X,\QQ). Maybe A(X,\QQ) could be used for the Chow ring. Or H^*(X,\QQ) for cohomology and H_*(X,\QQ) for the Chow ring (if intersection homology and the Chow ring were already identified somehow).

vbraun commented 12 years ago
comment:7

Oh yes good catch I meant to write A(X,\QQ) of course.

vbraun commented 12 years ago
comment:8

Fixed!

novoselt commented 12 years ago
comment:9

Comments 2&3 are still applicable and the patchbot complains ;-)

novoselt commented 12 years ago

Changed reviewer from Andrey Novoseltsev to David Eklund, Andrey Novoseltsev

vbraun commented 12 years ago

Attachment: trac_12361_fix_toric_cohomology_ring.patch.gz

Updated patch

vbraun commented 12 years ago
comment:10

I've improved the docstrings.

As for the whitespace, I think this is a non-issue. There was some discussion on sage-devel and the consensus seems to be that its not worth the effort. Its a button press away (either with emacs or with the mercurial checkfiles plugin) to strip all trailing whitespace, but that would just break every patch we currently have. Mercurial tells me that there are 170 places in toric_varieties.py with superfluous spaces, so a few more or less doesn't matter. I'd rather not spend an hour fixing all patches that I currently have just to make the whitespace plugin happy.

novoselt commented 12 years ago

Changed work issues from whitespaces and typos to none

jdemeyer commented 12 years ago

Description changed:

--- 
+++ 
@@ -2,4 +2,4 @@

 It turns out that index_in_saturation does not work for trivial zero-size matrices, this is also fixed.

-Apply  trac_12361_fix_toric_cohomology_ring.patch, trac_12361_index_in_saturation_fix.patch
+Apply [attachment: trac_12361_fix_toric_cohomology_ring.patch](https://github.com/sagemath/sage-prod/files/10654623/trac_12361_fix_toric_cohomology_ring.patch.gz), [attachment: trac_12361_index_in_saturation_fix.patch](https://github.com/sagemath/sage-prod/files/10654622/trac_12361_index_in_saturation_fix.patch.gz)
jdemeyer commented 12 years ago

Merged: sage-5.1.beta0