Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix normalization in cohomology ring of orbifold toric varieties #12361

Closed
vbraun opened this issue Jan 26, 2012 · 17 comments
Closed

Fix normalization in cohomology ring of orbifold toric varieties #12361

vbraun opened this issue Jan 26, 2012 · 17 comments

Comments

@vbraun
Copy link
Member

vbraun commented Jan 26, 2012

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
Copy link
Member Author

vbraun commented Jan 26, 2012

Attachment: trac_12361_index_in_saturation_fix.patch.gz

Initial patch

@vbraun

This comment has been minimized.

@novoselt
Copy link
Member

Reviewer: Andrey Novoseltsev

@novoselt
Copy link
Member

comment:2

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

@novoselt
Copy link
Member

comment:3

That's a really great expansion of the documentation!

I think I caught some typos:

  • In toric_variety line 213 should be "many quantities of interest" without "s" on the end.
  • On line 1977 "It gets tricky none of the maximal cones is smooth." does not sound right, some words or punctuation sighs are missing.

@novoselt
Copy link
Member

Work Issues: whitespaces and typos

@novoselt
Copy link
Member

comment:4

OK, other than the above looks good!

@sagetrac-davideklund
Copy link
Mannequin

sagetrac-davideklund mannequin commented Mar 28, 2012

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
Copy link
Member Author

vbraun commented Mar 28, 2012

comment:7

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

@vbraun
Copy link
Member Author

vbraun commented Mar 29, 2012

comment:8

Fixed!

@novoselt
Copy link
Member

comment:9

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

@novoselt
Copy link
Member

Changed reviewer from Andrey Novoseltsev to David Eklund, Andrey Novoseltsev

@vbraun
Copy link
Member Author

vbraun commented Mar 30, 2012

Attachment: trac_12361_fix_toric_cohomology_ring.patch.gz

Updated patch

@vbraun
Copy link
Member Author

vbraun commented Mar 30, 2012

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
Copy link
Member

Changed work issues from whitespaces and typos to none

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer modified the milestones: sage-5.0, sage-5.1 Apr 19, 2012
@jdemeyer
Copy link

jdemeyer commented May 6, 2012

Merged: sage-5.1.beta0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants