Opened 12 years ago

Closed 11 years ago

#9713 closed enhancement (fixed)

Add toric Chow group

Reported by: vbraun Owned by: AlexGhitza
Priority: major Milestone: sage-4.6.2
Component: algebraic geometry Keywords:
Cc: novoselt Merged in: sage-4.6.2.alpha4
Authors: Volker Braun Reviewers: Andrey Novoseltsev, Simon King, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

The toric Chow group is a useful version of a homology theory for toric varieties, especially for singular varieties. The attached patch provides support for computing the toric Chow group as well as elementary intersection theory.

See tracker bug at #9604 to for related patches.

Apply:

  1. trac_9713_fix_cardinality.patch
  2. trac_9713_fix_fg_pid.2.patch
  3. trac_9713_fix_fg_pid_reviewer.2.patch
  4. trac_9713_toric_chow_group.patch
  5. 9713_hash.patch

Depends on #8948.

Attachments (9)

trac_9713_reviewer.patch (8.3 KB) - added by novoselt 11 years ago.
trac_9713_fix_fg_pid_relative_10513.patch (41.3 KB) - added by SimonKing 11 years ago.
Rebases trac_9713_fix_fg_pid.patch relative to #10513
trac_9713_fix_fg_pid.patch (48.0 KB) - added by vbraun 11 years ago.
Renamed element_class -> Element
trac_9713_fix_fg_pid_reviewer.patch (8.1 KB) - added by SimonKing 11 years ago.
Apply after trac_9713_fix_fg_pid.patch: Element vs. element_class; indirect doctests
trac_9713_fix_fg_pid_reviewer.2.patch (8.1 KB) - added by vbraun 11 years ago.
Rebased against sage-4.6.2.alpha2
trac_9713_fix_cardinality.patch (3.7 KB) - added by vbraun 11 years ago.
Rebased against sage-4.6.2.alpha2
trac_9713_fix_fg_pid.2.patch (48.0 KB) - added by vbraun 11 years ago.
Rebased patch.
9713_hash.patch (741 bytes) - added by jdemeyer 11 years ago.
trac_9713_toric_chow_group.patch (48.4 KB) - added by vbraun 11 years ago.
Updated patch

Download all attachments as: .zip

Change History (85)

comment:1 Changed 12 years ago by novoselt

  • Status changed from new to needs_info

Volker, is this one ready for review? (It needs a little rebasement on top of new #9337.)

comment:2 Changed 12 years ago by vbraun

  • Reviewers set to Andrey Novoseltsev
  • Status changed from needs_info to needs_review

I think its ready, but you will probably soon find something to disagree on ;-)

Rebased patch is attached.

comment:3 Changed 12 years ago by novoselt

  • Status changed from needs_review to needs_work

Some preliminary comments:

  1. The new module is not completely documented yet, sage -coverage shows some issues.
  2. It seems to me that it is more common to use Ak rather than Ad-k, so I think it would be better to stick with it in the documentation.
  3. I'd prefer to drop "The " from "The Chow cycle ..." in _repr_ as was done for divisor classes.
  4. It is not clear how to construct Chow cycles (except for those that correspond to cones).
  5. It is not clear what do numbers mean in the representation - i.e. how the basis is chosen?
  6. Regardless of the chosen basis, I think it would be more useful to see cycles as linear combination of cones (perhaps with cones represented by their ambient_ray_indices), without seeing those that got zero coefficients, of course. This issue is also relevant for divisor/cohomology classes and for divisor classes we do use coordinates in some basis, but I think that it makes more sense there, while here basis elements can be given by cycles of different dimension and having no separation between them makes it difficult to interpret the output.

comment:4 Changed 12 years ago by vbraun

I'll try to add the missing docstrings.

I was intentionally using A_k and not A^{d-k} because this patch implements the Chow homology and not the cohomology. I'm not sure to which extend they are the same over singular/noncompact varieties over ZZ.

I'm not entirely happy with the output either. But printing all the cones (even if we abbreviate them) will very quickly produce multi-line output. Right now, the output is essentially in a random basis corresponding to cycle.parent().degree().

sage: cycle = X.Chow_group().gen(1)
sage: cycle
The Chow cycle (0, 1, 0, 0, 0, 0)
sage: cycle.parent().degree()
(Z, Z x Z, Z x Z, Z)
sage: cycle.lift()
(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0)

For all practical purposes, the user should use lift() to get the coefficients of the cones in the given order flatten(toric_variety.fan().cones()). Would printing

Chow cycle ((0,), (1, 0), (0, 0), (0,))

make you more happy? Note that some methods (in particular, intersection with a divisor) need to change the cycle within its Chow class, so even "simple" cycles will produce output where all coefficients are turned on. This is part of why I don't want to print the cone information explicitly.

comment:5 Changed 12 years ago by novoselt

OK, let's stick to A_k.

Chow cycle ((0,), (1, 0), (0, 0), (0,))

is a bit ugly ;-) Maybe to have it as a vector but separate degrees something like this may work:

Chow cycle (0 | 1, 0 | 0, 0 | 0)

My concern is that if I create the Chow cycle corresponding to a cone generated by rays 2 and 5, it would be nice to see it clearly in the output, say

Chow cycle {2, 5}

with combinations printed as

Chow cycle 2*{2, 5} - 7*{1, 2, 3}

But to have it nicely will require storing the original cone combination, since lift algorithm from a quotient element will involve a lot of cones, as you pointed out. I don't know if it worths the benefits. But it would be nice at least to have some markers between degrees.

comment:6 Changed 12 years ago by vbraun

I've changed the printing of Chow cycles to the following:

sage: X = toric_varieties.Cube_deformation(7)
sage: A = X.Chow_group()
sage: A.degree()
(Z, C7, Z^5 x C2 x C2, Z)
sage: a = sum( A.gen(i) * (i+1) for i in range(0,A.ngens()) )   # an element of A
sage: a
( 3 | 1 mod 7 | 0 mod 2, 1 mod 2, 4, 5, 6, 7, 8 | 9 )

I'll upload a new patch when #9972 is finished, as this patch will require some rediffing.

comment:7 Changed 11 years ago by vbraun

  • Status changed from needs_work to needs_review

Updated patch now applies cleanly. Ready for review...

comment:8 Changed 11 years ago by novoselt

  • Status changed from needs_review to needs_work
  • Work issues set to coordinate order of components

I am a bit confused by the first example:

sage: A.degree()
(Z, C7, Z^5 x C2 x C2, Z)
sage: a = sum( A.gen(i) * (i+1) for i in range(0,A.ngens()) )   # an element of A
sage: a
( 3 | 1 mod 7 | 0 mod 2, 1 mod 2, 4, 5, 6, 7, 8 | 9 )

with explanation "The Chow group elements are printed as ( a0 | a1 mod 7 | a2 mod 2, a3 mod 2, a4, a5, a6, a7, a8 | a9 ), which denotes the element of the Chow group in the same basis as A.degree()."

A.degree() shows the third component as Z^5 x C2 x C2, yet it seems that elements put cyclic groups first, or maybe it is the reverse order (which would matter for several cyclic groups of different degrees).

comment:9 Changed 11 years ago by novoselt

A little optional note: it may be better to use \QQ etc. rather than \mathbb{Q}. I personally prefer the look of the last variant, but using standard macros brings more uniformity to the whole Sage documentation.

comment:10 Changed 11 years ago by vbraun

  • Status changed from needs_work to needs_review

Done. Now prints as

sage: A.degree()
(Z, C7, C2 x C2 x Z^5, Z)

because thats how FGP_Module organizes its generators (first torsion, then free generators).

comment:11 Changed 11 years ago by kcrisman

Just a minor point - there is a lot of reference to the Chow group of this and that, but it's apparently only the 'toric' Chow group, as noted at the beginning of the file. Is it typical in the literature to conflate these notions - or is there a theorem that they are equivalent for toric varieties? This should be clarified somehow, maybe even with an example of where they are not the same. It would be unfortunate if they were *not* the same and someone later wanted to implement regular Chow groups!

Forgive me if the question is naive; I am fairly familiar with 'regular' Chow groups but hadn't encountered this (natural) version before. If there is a foundational paper, it would be helpful to refer to that in the documentation as well; the Wikipedia page referenced doesn't actually refer to the 'toric' variety at all.

comment:12 follow-up: Changed 11 years ago by vbraun

The toric Chow cycles are the torus-invariant subvarieties only. Its a standard construction, see e.g. Fultons book. I don't think that there is any hope of it being the same as the full Chow group. But then the Chow group is in general huge and there is little hope of a complete description, and even less of a toric algorithm. I guess we could sprinkle some more "toric_" prefixes around in anticipation of someone inventing the requisite mathematics. But I think its clear enough...

comment:13 in reply to: ↑ 12 Changed 11 years ago by kcrisman

Replying to vbraun:

The toric Chow cycles are the torus-invariant subvarieties only.

Yes, I got that.

Its a standard construction, see e.g. Fultons book. I don't think that there is any hope of it being the same as the full Chow group.

Ah, when you say "Fulton's book" I first got out Intersection Theory :) but here it is. Sections 3.3 and 3.4 seem quite relevant. Page 63 says that Pic and the divisor piece of the Chow group can be computed only with what Fulton consistently calls "T-Weil" and "T-Cartier" divisors. Further, the first proposition in Chapter 5 certainly seems to imply that this is in fact true in general for toric varieties. The 'orbit closures' seem to be toric subvarieties by definition, and they generate the Chow group. Am I reading this wrong?

comment:14 Changed 11 years ago by vbraun

The T-Weil divisor class group is the divisor part of the toric Chow group, and the T-Cartier divisor class group is the Pic (not the other way round). And the orbit closures are the toric subvarieties. As usual, the divisor part of the Chow group is easy; the higher Chow groups become more complicated. Once you understand these, the Hodge conjecture follows easily ;-)

comment:15 Changed 11 years ago by kcrisman

Yes, I didn't intend to switch those but did by accident. Sorry.

Maybe I wasn't clear. I'm saying that the propositions I reference seem to imply that the regular Chow groups ARE the toric Chow groups, because the free Abelian group on p-cycles turns out to be generated by the toric p-cycles in a toric subvariety. But again, maybe I'm misreading them somehow - are there fewer relations? But the definitions there seem to have the same notion of rational equivalence (?). Unfortunately, I don't have it on me, it was at work I checked this out.

Oh, and don't call them higher Chow groups; that is something else again :) unless you want to start talking about the so-called Beilinson-Hodge conjecture

comment:16 Changed 11 years ago by novoselt

  • Work issues coordinate order of components deleted

Here it is, I called the fan Sigma rather than Delta:

Proposition The Chow group A_k_(X) of an arbitrary toric variety X = X(Sigma) is generated by the classes of the orbit closures V(sigma) of the cones of dimension n-k of Sigma.

So yes, the patch does implement the "honest Chow group" with the exception that arbitrary subvarieties cannot be translated into their Chow cycles (I think).

Karl, do you plan to review the patch completely? It is fine if no, I am going to do it in the near future. But if yes, it certainly will be great to have more people looking at our toric stuff ;-)

comment:17 Changed 11 years ago by kcrisman

Yes, I think that is the case - nicely summed up.

No, I am not at all an expert on toric varieties (though I wish I had learned more about them in graduate school now, they are so concrete), so I can't help review. The title of the ticket was just too good not to click on, and then I had this minor point which ballooned into a conversation. Since of course lots of stuff IS toric that one might care about, I am glad you are doing it, but I'm sorry that it's above my pay grade for now. If I had a sabbatical right now we might be talking ;)

comment:18 Changed 11 years ago by vbraun

By "higher", I mean of course "lower degree" Chow groups. I'm pretty sure its correct in the module documentation :-)

But the free Abelian group of p-cycles is not generated by the toric p-cycles; The former is never finitely generated while the latter always is. In the case of divisors, the relations save you (the Pic^0 is a point). But already for the curves part of the Chow group of a three-dimensional variety its not clear to me that the full Chow group is finitely generated.

comment:19 Changed 11 years ago by novoselt

Well, Fulton claims that quotients are the same even though the starting free groups are very different ;-)

So I think we should include this proposition in the top of the module level documentation and comment that this allows us to worry only about orbit closures.

comment:20 Changed 11 years ago by vbraun

The relevant paper is Fulton/MacPherson/Sottile/Sturmfels?: Intersection theory on spherical varieties. The bottom line is that, for any connected solvable linear algebraic group G acting on a scheme, the G-Chow group (made out of G-invariant cycles) is canonically the same as the ordinary Chow group. I think I knew this paper at one point, but then obviously forgot ;-)

comment:21 Changed 11 years ago by novoselt

  • Status changed from needs_review to needs_work
  1. The discussed above fact that this patch implement the full Chow group should be reflected in module level documentation.
  2. Documentation from ChowCycle.__init__ should be moved to the class docstring, otherwise it is invisible.
  3. Behaviour of check=False is strange for it: usually such an option omits validity checks assuming that user will take care of it. Here it seems to allow a different form of input. Is it inherited from the base class? If so, maybe it should be fixed there...
  4. Line 170 misses the second ":" in the end.
  5. The first words of some docstrings have extra s'es, like "Returns" and "Intersects".
  6. It would be nice if the documentation and example for count_points were expanded/clarified a little bit more. The phrase "integral over the dual cohomology class" does not make it clear what is the integrand and to what the cohomology class is dual. Is it the Chow cycle in both cases? The example starts with a divisor and then integrates the square of this divisor and counts points on the intersection of the Chow cycle of this divisor with the original divisor. How about starting with some Chow cycle with non-zero point count and then showing how to get a corresponding cohomology class and integrate it?
  7. For intersect_with_divisor it would be nice to give a reference to the used algorithm (Fulton p.97?). I think also that intersection_with_divisor would be a better name, since the function returns the intersection rather than updates the original cycle. (I do realize now that the same consideration should have been applied to some methods that I have added...) By the way, this function has no input/output description. And perhaps i = I_gamma.pop() does the same thing as i = iter(I_gamma).next() in a little cleaner fashion. (It does alter the set, but it is not used anyway.) What exactly is tested by the long test with many zeros in this function? This function says that divisor must be Cartier, but it is not checked, is it intended? Am I right that it actually makes sense to use it with Q-Cartier divisors if the Chow group is also rational?
  8. Chow_cycle method of toric divisors does not have a doctest and input/output description.
  9. Line 394 misses closing quotes after True.

I didn't go over actual Chow group code yet, but will do it. Soon ;-)

comment:22 Changed 11 years ago by novoselt

  1. It is probably worth mentioning what is the point of A(Cone(cone)) after A(cone). I actually didn't even know that Cone(cone) works! Fortunately, it works as expected ;-)
  2. In _rational_equivalence_relations I find it very difficult to understand the LaTeX expression, which does not get typeset in the documentation (since it is a private method), especially since it does not explain what is "! over =", what is n_{\rho, \sigma}, and how d, k, n, and p are related (I guess pairs of them are equal). I think that this description should be moved to some visible place (module documentation or relation_gens?) and there should be a reference to where it was taken from.
  3. What is the purpose of the super call in __eq__ for Chow groups? Do we want anything but a Chow group equal to a Chow group?
  4. I think that _cone_to_V would be more efficient if it was implemented via a dictionary. I don't know how significant such a speed up will be in real computations, but I think it is worthwhile to do this change.
  5. There is a note in degree method about smooth varieties: "Using the cohomology ring instead of the Chow group will be much faster." I think that this is a very important point and it should be also mentioned in the module level documentation and in the Chow group method of toric varieties. (Perhaps with explanations of why this is the case.) Examples of this method are very nice! I'd remove the sentence "The resulting fan is not the fan over a convex cube."
  6. I think it would be more natural if gens without parameters returned the union of all gens of specified degree. Is it possible to implement? (I.e. is it possible to force the quotient module to use these generators? I definitely don't suggest "just" returning union of degree generators.)
  7. For relation_gens wouldn't it be more natural to return elements of the covering module, i.e. lifts of the current output?
  8. Line 979: you meant == QQ.
  9. Would it make sense to derive ChowGroup_degree_class from some kind of a group rather than SageObject?

That's it for the current patch!

comment:23 Changed 11 years ago by novoselt

One more global question: did you think about having most of the functionality in ChowGroup_degree_class with Chow cycles being tuples of elements from all of them, i.e. without having the total quotient module? That would take care of some of the issues above (like gens vs. gens(degree=k)) and would avoid flatten(fan.cones()) which I don't really like. It would also make sense to let fans determine indices of the cones in the lists in this case, i.e. fan._index_of(cone) would return the index of cone in fan.cones(dim=cone.dim()).

comment:24 Changed 11 years ago by vbraun

When I wrote the code I tried to make gens() return the union of the the fixed-degree generators but that did not work. I don't remember what exactly went wrong, but bad things happen if gens() is different from the generators that FGP_Module choses.

I mostly added ChowGroup_degree_class so that the individual degree pieces can _repr_() themselves nicely. One could make them parents again with some coercion in the full Chow group, but that seems to be a big effort. I definitely want to allow mixed-degree Chow group elements, so we can't get rid of the whole Chow group.

comment:25 Changed 11 years ago by vbraun

A1. The discussed above fact that this patch implement the full Chow group should be reflected in module level documentation.

OK

A2. Documentation from ChowCycle.__init__ should be moved to the class docstring, otherwise it is invisible.

I've added a note to the ChowCycle class documentation that you are not supposed to manually construct them. The __init__ docstring is hidden on purpose.

A3. Behaviour of check=False is strange for it: usually such an option omits validity checks assuming that user will take care of it. Here it seems to allow a different form of input. Is it inherited from the base class? If so, maybe it should be fixed there...

Yes, it is inherited from FGP_Element. I tried to fix it there, but then I ended up trying to rewrite it for the new coercion model. And that attempt was quite unsuccessful, all of sage.modules is interrelated and you can't just change one class...

A4. Line 170 misses the second ":" in the end.

OK

A5. The first words of some docstrings have extra s'es, like "Returns" and "Intersects".

OK

A6. It would be nice if the documentation and example for count_points were expanded/clarified a little bit more. The phrase "integral over the dual cohomology class" does not make it clear what is the integrand and to what the cohomology class is dual. Is it the Chow cycle in both cases? The example starts with a divisor and then integrates the square of this divisor and counts points on the intersection of the Chow cycle of this divisor with the original divisor. How about starting with some Chow cycle with non-zero point count and then showing how to get a corresponding cohomology class and integrate it?

There is no easy way to get the Poincare dual (In fact, it exists only if the variety is smooth). I've clarified the example a bit.

A7. For intersect_with_divisor it would be nice to give a reference to the used algorithm (Fulton p.97?). I think also that intersection_with_divisor would be a better name, since the function returns the intersection rather than updates the original cycle. (I do realize now that the same consideration should have been applied to some methods that I have added...) By the way, this function has no input/output description. And perhaps i = _gamma.pop() does the same thing as i = iter(I_gamma).next() in a little cleaner fashion. (It does alter the set, but it is not used anyway.) What exactly is tested by the long test with many zeros in this function? This function says that divisor must be Cartier, but it is not checked, is it intended? Am I right that it actually makes sense to use it with Q-Cartier divisors if the Chow group is also rational?

I renamed it to intersection_with_divisor() and expanded on the documentation which should now answer your questions...

In principle you are right with the Q-Cartier divisors, but in practice this will not work because multiplication QQ * QQ-Chow cycle is broken. The problem is that the parent does not adhere to the new coercion model. I could hack around it, but then this should really be fixed elsewhere.

A8. Chow_cycle method of toric divisors does not have a doctest and input/output description.

OK

A9. Line 394 misses closing quotes after True.

OK

B1. It is probably worth mentioning what is the point of A(Cone(cone)) after A(cone). I actually didn't even know that Cone(cone) works! Fortunately, it works as expected ;-)

OK

B2. In _rational_equivalence_relations I find it very difficult to understand the LaTeX expression, which does not get typeset in the documentation (since it is a private method), especially since it does not explain what is "! over =", what is n_{\rho, \sigma}, and how d, k, n, and p are related (I guess pairs of them are equal). I think that this description should be moved to some visible place (module documentation or relation_gens?) and there should be a reference to where it was taken from.

I moved it to relation_gens and added more explanations.

B3. What is the purpose of the super call in __eq__ for Chow groups? Do we want anything but a Chow group equal to a Chow group?

Now it only returns self is other.

B4. I think that _cone_to_V would be more efficient if it was implemented via a dictionary. I don't know how significant such a speed up will be in real computations, but I think it is worthwhile to do this change.

Its only constructing a unit vector. Surely the run time for intersection computations is dominated by the matrix normal form computations and not the construction of unit vectors?

B5. There is a note in degree method about smooth varieties: "Using the cohomology ring instead of the Chow group will be much faster." I think that this is a very important point and it should be also mentioned in the module level documentation and in the Chow group method of toric varieties. (Perhaps with explanations of why this is the case.) Examples of this method are very nice! I'd remove the sentence "The resulting fan is not the fan over a convex cube."

OK

B6. I think it would be more natural if gens without parameters returned the union of all gens of specified degree. Is it possible to implement? (I.e. is it possible to force the quotient module to use these generators? I definitely don't suggest "just" returning union of degree generators.)

I tried it but couldn't get it to work. The base FGP_Module has no provision to explicitly set the generators, and bad things happen if they are different from the internal normal form of the basis matrix.

B7. For relation_gens wouldn't it be more natural to return elements of the covering module, i.e. lifts of the current output?

You can do that with A.relations().gens(). I added a note to relation_gens.

B8. Line 979: you meant == QQ.

Yes, fixed.

B9. Would it make sense to derive ChowGroup_degree_class from some kind of a group rather than SageObject?

Ideally they would be some other parents with coercions into the full (mixed-degree) Chow group. But then thats a lot of work for very little benefit. Not to mention that the base FGP_Module does not adhere to the new coercion model, so it'll be hackisch...

comment:26 Changed 11 years ago by novoselt

  • Milestone changed from sage-4.6.1 to sage-4.6.2
  • Status changed from needs_work to needs_review

For the buildbot: Depends on #9972 and #10325

comment:27 Changed 11 years ago by vbraun

  • Status changed from needs_review to needs_work

Wait, I'm still meddling with the coercions...

comment:28 Changed 11 years ago by novoselt

Replying to vbraun:

A3. OK, let's hope it will get fixed eventually...

A6. Am I right that it is integral OF the dual cohomology class, not OVER? That's how I interpret integrate method of toric varieties. Also it seems that we need completeness in addition to smoothness.

A7. Let it be fixed elsewhere!

B9. Ok, let it be as is.

Fresh comments:

  1. Why get_degree and not just degree for Chow cycles?
  2. What will cohomology_class of a Chow cycle produce in the case when there is no Poincare duality? I think this should be either explained in the documentation, or an exception should be raised varieties which are not smooth and complete.

I have done some documentation prettification, feel free to fold it into your patch, if is still applies to your current version.

Changed 11 years ago by novoselt

comment:29 follow-up: Changed 11 years ago by vbraun

A6. Yes, integral "of". Will fix it ;-)

  1. ChowGroup.degree() is probably a stupid name... ideas for better names? But I wanted to distinguish it from ChowCycle.degree().
  1. There is no ChowCycle.cohomology_class() method nor any coercion to the CohomologyRing.

comment:30 in reply to: ↑ 29 Changed 11 years ago by novoselt

Replying to vbraun:

  1. ChowGroup.degree() is probably a stupid name... ideas for better names? But I wanted to distinguish it from ChowCycle.degree().

I meant that ChowCycle.degree() is better than ChowCycle.get_degree() defined on line 243. It seems fine to me, why don't you like it?

  1. There is no ChowCycle.cohomology_class() method nor any coercion to the CohomologyRing.

What about line 444?..

comment:31 Changed 11 years ago by vbraun

Oops I totally forgot that I implemented the cohomology_class() method :-)

I ported the bases Module and FGP_Module over to the new coercion model. The patch touches a lot of files but mostly small fixes, only sage/modules/module.py and sage/modules/fg_pid/*py are modified big time.

For the tracbot: Apply trac_9713_fix_cardinality.patch, trac_9713_fix_fg_pid.patch, trac_9713_toric_chow_group.patch

comment:32 Changed 11 years ago by vbraun

  • Status changed from needs_work to needs_review

comment:33 Changed 11 years ago by SimonKing

  • Status changed from needs_review to needs_work

I am about to create a new ticket whose purpose is to implement the new parent and coercion framework not only for the base class sage.modules.module.Module, but also for the most derived classes. In that regard, it goes beyond what is done in the patches here.

My to-be-created ticket will leave the FGP_* stuff almost unchanged. I suggest to include that to-be-created ticket as a new dependency (mentioning it in #9604 as well), so that the ticket here can concentrate on its "real" purpose, which is not coercion but toric Chow group.

Then I would also provide a replacement of trac_9713_toric_chow_group.patch relative to the to-be-created ticket.

A few words on trac_9713_toric_chow_group.patch: The element_class attribute of Parent is in fact a lazy attribute, that takes another attribute Element and mixes it with some category stuff before it turns element_class into an actual attribute. Hence, one should not directly set element_class (which is done in the patch), but should provide an attribute Element instead.

My to-be-created patch would actually preserve the _element_class (leading underscore) method, since it chooses the right element class and is already implemented for all sub-classes. But I would call it in the __init__ method of the base class sage.modules.module.Module and assign its output to the Element attribute. In that way, it is most easy to implement the lazy element_class (no leading underscore) attribute as one is supposed to.

Also, the __call__ method of fgp morphisms should probably be replaces by _call_, _call_with_args and perhaps pushforward (likely depending on #10496).

Best regards, Simon

Changed 11 years ago by SimonKing

Rebases trac_9713_fix_fg_pid.patch relative to #10513

comment:34 Changed 11 years ago by SimonKing

  • Status changed from needs_work to needs_review

I just attached a version of trac_9713_fix_fg_pid.patch that is based on #10513. I'll add #10513 to the dependencies that are listed in #9604.

I can not vouch for any passing doc tests, as I merely rebased the patch.

comment:35 Changed 11 years ago by SimonKing

Sorry! Since #10513 needs much work, I should not make it a dependency for the ticket here. So, please disregard the patch trac_9713_fix_fg_pid_relative_10513.patch!

I understood that you just want to implement the parent and category framework to an extent that allows you to implement the toric Chow group. Right? Then I think my remark on _call_ and pushforward and _call_with_args of morphisms should be moved to a different ticket.

So, it remains "needs review".

Cheers, Simon

comment:36 follow-up: Changed 11 years ago by novoselt

  • Description modified (diff)

Just to make it clear: so far this ticket depends on #9972 and #10325 (both positively reviewed) on top of sage-4.6.1.alpha3, nothing else.

comment:37 in reply to: ↑ 36 Changed 11 years ago by SimonKing

Replying to novoselt:

Just to make it clear: so far this ticket depends on #9972 and #10325 (both positively reviewed) on top of sage-4.6.1.alpha3, nothing else.

Thank you for the clarification! I thought I had to apply all patches mentioned at #9604.

Changed 11 years ago by vbraun

Renamed element_class -> Element

comment:38 follow-ups: Changed 11 years ago by vbraun

I've renamed element_class -> Element as per Simon's comment, thanks!

Maybe Simon would be interested in reviewing the first two patches (trac_9713_fix_cardinality.patch, trac_9713_fix_fg_pid.patch) and Andrey the trac_9713_toric_chow_group.patch? :-) Then we should make it into Sage-4.6.2...

comment:39 in reply to: ↑ 38 Changed 11 years ago by SimonKing

Replying to vbraun:

I've renamed element_class -> Element as per Simon's comment, thanks!

Maybe Simon would be interested in reviewing the first two patches (trac_9713_fix_cardinality.patch, trac_9713_fix_fg_pid.patch) and Andrey the trac_9713_toric_chow_group.patch? :-) Then we should make it into Sage-4.6.2...

In what order should the patches be applied? I first applied the patches from #9972 and #10325, then proceeded with trac_9713_toric_chow_group.patch, but one hunk of trac_9713_fix_fg_pid.patch failed.

comment:40 follow-up: Changed 11 years ago by vbraun

See also comment 31:

  • trac_9713_fix_cardinality.patch
  • trac_9713_fix_fg_pid.patch
  • trac_9713_toric_chow_group.patch

comment:41 in reply to: ↑ 40 ; follow-up: Changed 11 years ago by SimonKing

Replying to vbraun:

See also comment 31:

  • trac_9713_fix_cardinality.patch
  • trac_9713_fix_fg_pid.patch

No, when I apply trac_9713_fix_fg_pid.patch, one out of 17 hunks fails, namely:

--- fgp_module.py
+++ fgp_module.py
@@ -539,7 +578,8 @@
         """
         return other.is_submodule(self)

-    def __call__(self, x, check=True):
+
+    def _element_constructor_(self, x, check=True):
         """
         INPUT:

Cheers, Simon

comment:42 follow-up: Changed 11 years ago by vbraun

Works for me, which sage version are you trying to apply it to?

/home/vbraun/opt/sage-4.6.1.rc0/devel/sage
[vbraun@volker-desktop sage]$ hg qpush -a
applying trac_9972_add_cone_embedding.patch
applying trac_9972_improve_element_constructors.patch
applying trac_9972_remove_enhanced_cones_and_fans.patch
applying trac_9972_add_fan_morphisms.patch
applying trac_9972_fix_fan_warning.patch
applying trac_10325_make_toric_CohomologyRing_unique.patch
applying trac_9713_fix_cardinality.patch
applying trac_9713_fix_fg_pid.patch
applying trac_9713_toric_chow_group.patch
now at: trac_9713_toric_chow_group.patch

comment:43 in reply to: ↑ 41 Changed 11 years ago by SimonKing

Replying to SimonKing:

No, when I apply trac_9713_fix_fg_pid.patch, one out of 17 hunks fails

It seems to me that this hunk has a totally wrong line number. The line

    def __call__(self, x, check=True):

is line number 446 in fgp_module.py.

comment:44 Changed 11 years ago by vbraun

No the __call__ in FGP_Module is at line 542. Maybe you forgot to revert something in your sage library?

[vbraun@volker-desktop sage]$ hg qpop -a
no patches applied
[vbraun@volker-desktop sage]$ head -542 sage/modules/fg_pid/fgp_module.py | tail -1
    def __call__(self, x, check=True):
[vbraun@volker-desktop sage]$ 

comment:45 in reply to: ↑ 42 Changed 11 years ago by SimonKing

Replying to vbraun:

Works for me, which sage version are you trying to apply it to?

sage-4.6

So, perhaps I should try to upgrade to sage-4.6.1.alpha3 first.

Is there an easy way to use sage -upgrade for a development version?

comment:46 follow-up: Changed 11 years ago by vbraun

I usually build from scratch, but I believe the following should work:

sage -upgrade http://sage.math.washington.edu/home/release/sage-4.6.1.rc0/sage-4.6.1.rc0/

comment:47 in reply to: ↑ 46 Changed 11 years ago by SimonKing

Replying to vbraun:

I usually build from scratch, but I believe the following should work:

Thanks, it did kind of work. At some point the process got stuck, but when I then did make in SAGE_ROOT, the upgrade did succeed (at least so it seems).

The patches apply. I don't know if I will be able to do reviewing any time soon, since currently I only have remote access to my computer via rather slow internet (237 kB/s).

Cheers, Simon

comment:48 Changed 11 years ago by SimonKing

  • Reviewers changed from Andrey Novoseltsev to Andrey Novoseltsev, Simon King
  • Status changed from needs_review to needs_work
  • Work issues set to element class

I applied the patches on top of sage-4.6.1.alpha3, and the good news is that all tests pass.

However, I have some criticism concerning trac_9713_fix_fg_pid.patch.

In several cases I see things like

        return self.parent().Element(self.parent(), self._x - other._x)

That's not how the attribute Element is supposed to be used. As far as I know, the story is like this:

  • One provides an attribute Element, which is supposed to be a class.
  • Parent.element_class is a so-called lazy attribute. That's to say, if one does self.element_class then the method self.element_class() is called, which computes a new class out of self.Element and self.category(), assigns this new class to an actual (not lazy) attribute self.element_class, and returns that new class.
  • When one requests self.element_class is called the next time then it is already a usual attribute, which is of course much faster than calling a method.

Hence, you should define self.Element. But then you should do

        P = self.parent()
        return P.element_class(P, self._x - other._x) 

since P.element_class (in contrast to P.Element) also inherits stuff from the category. Note that additionally the code snipped spares a little time compared with your patch since self.parent() is called only once instead of twice.

As I mentioned, my bandwith is a bit restricted at the moment. So, this post might be incomplete, it is just what I found by a little reading of the patch.

Changed 11 years ago by SimonKing

Apply after trac_9713_fix_fg_pid.patch: Element vs. element_class; indirect doctests

comment:49 in reply to: ↑ 38 Changed 11 years ago by SimonKing

  • Status changed from needs_work to needs_review

Hi Volker and Andrey!

Replying to vbraun:

I've renamed element_class -> Element as per Simon's comment, thanks!

As I've pointed out in my previous post, one should define self.Element, but then the actual element class is self.element_class. This is taken care of in my reviewer patch, trac_9713_fix_fg_pid_reviewer.patch. Some doctests needed to be modified accordingly. The reviewer patch also raises the doctest coverage of sage.modules.fg_pid to 100% (some indirect tests had not been marked as such).

Maybe Simon would be interested in reviewing the first two patches (trac_9713_fix_cardinality.patch, trac_9713_fix_fg_pid.patch) and Andrey the trac_9713_toric_chow_group.patch?

I give trac_9713_fix_cardinality.patch and trac_9713_fix_fg_pid.patch (modulo my reviewer patch) a positive review. Of course, we all know that it does not provide a full implementation of the new parent and coercion model for modules; after all, that is not the purpose of this ticket. But apparently it is enough for implementing the toric chow group. So, I suggest to build a full implementation of coercion for modules on top of these patches.

Then we should make it into Sage-4.6.2...

My job is done. Andrey, your turn... :)

comment:50 Changed 11 years ago by SimonKing

  • Work issues element class deleted

comment:51 Changed 11 years ago by vbraun

Thank you for the element_class explanation! I'm happy with the reviewer patch. I've updated the remaining (and final) patch to also use element_class in the Chow group, so we should be all set. Andrey, can you set this ticket to positive review?

For the tracbot: Apply trac_9713_fix_cardinality.patch, trac_9713_fix_fg_pid.patch, trac_9713_fix_fg_pid_reviewer.patch, trac_9713_toric_chow_group.patch

comment:52 Changed 11 years ago by novoselt

Simon, thanks a lot for your help!

Volker, shouldn't variety be complete for Poincare duality in cohomology_class?

In its documentation "will be not always match intersection numbers." should be "will not always match intersection numbers." (or "may not match intersection numbers.")

Otherwise tests pass and all issues above seem to be resolved!

comment:53 Changed 11 years ago by novoselt

  • Status changed from needs_review to needs_info

So - do we need completeness and a check for it?-)

comment:54 Changed 11 years ago by vbraun

I've extended the docstring to clarify when cohomology_class() is Poincare dual and when not. The added example should also clarify things.

comment:55 Changed 11 years ago by novoselt

Can we rephrase "We can associate a degree-d polynomial in the homogeneous coordinates by taking the product of the variables corresponding to the rays." to something else? My concern is that these variables may have different weight and, in general, there is no Z-grading on a toric variety. Maybe "We can associate to it the product of homogeneous coordinates corresponding to its rays."?

Right before OUTPUT "will be not always" should be "will not always"

comment:56 Changed 11 years ago by vbraun

Ok I've rewritten it to be more careful with "degrees".

comment:57 Changed 11 years ago by novoselt

  • Status changed from needs_info to needs_review

comment:58 Changed 11 years ago by novoselt

  • Status changed from needs_review to positive_review

Hooray!

comment:59 Changed 11 years ago by jdemeyer

  • Description modified (diff)

comment:60 Changed 11 years ago by jdemeyer

  • Status changed from positive_review to needs_work
  • Work issues set to rebase

Needs to be rebased to sage-4.6.2.alpha2

Changed 11 years ago by vbraun

Rebased against sage-4.6.2.alpha2

Changed 11 years ago by vbraun

Rebased against sage-4.6.2.alpha2

comment:61 Changed 11 years ago by vbraun

  • Description modified (diff)
  • Status changed from needs_work to positive_review

Rebased against sage-4.6.2.alpha2

comment:62 Changed 11 years ago by novoselt

What was necessary in rebasing??? I still have my queue applied smoothly on alpha2!

comment:63 Changed 11 years ago by vbraun

  • Work issues rebase deleted

I haven't actually checked whether anything broke. I don't remember if I had to refresh any patches for 4.6.2.alpha2, I just believed Jeroen that there was a problem. In any case, can't hurt.

comment:64 Changed 11 years ago by jdemeyer

  • Description modified (diff)

comment:65 Changed 11 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Okay, there problem is slightly more involved: there is a conflict with #8948 (which is not yet merged in any alpha). For totally arbitrary reasons I've decided to merge #8948 first, so could you please rebase your patches (at least trac_9713_fix_fg_pid.2.patch) to apply on top of #8948?

Changed 11 years ago by vbraun

Rebased patch.

comment:66 follow-up: Changed 11 years ago by vbraun

  • Description modified (diff)

Rebased on top of Sage-4.6.2.alpha2+#8948.

comment:67 Changed 11 years ago by vbraun

  • Status changed from needs_work to positive_review

comment:68 in reply to: ↑ 66 Changed 11 years ago by jdemeyer

Replying to vbraun:

Rebased on top of Sage-4.6.2.alpha2+#8948.

Thanks! Patches apply fine now. Sorry for the mess.

comment:69 Changed 11 years ago by jdemeyer

  • Merged in set to sage-4.6.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:70 Changed 11 years ago by jdemeyer

  • Merged in sage-4.6.2.alpha3 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

A doctest like this will clearly break on 32-bit systems:

    def __hash__(self):
        """
        Return a hash value for the :class:`Module` instance.

        OUTPUT:

        An integer.

        EXAMPLES::

            sage: from sage.modules.module import Module
            sage: M = Module(ZZ)
            sage: M.__hash__()
            6190647798068218210
        """
        return hash(self.__repr__())

comment:71 Changed 11 years ago by jdemeyer

  • Status changed from new to needs_work

Changed 11 years ago by jdemeyer

comment:72 Changed 11 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers changed from Andrey Novoseltsev, Simon King to Andrey Novoseltsev, Simon King, Jeroen Demeyer
  • Status changed from needs_work to needs_review

comment:73 Changed 11 years ago by novoselt

  • Status changed from needs_review to positive_review

Thanks for catching this!

comment:74 Changed 11 years ago by jdemeyer

  • Status changed from positive_review to needs_work

I'm afraid I have to bother you again. With this patch, the pdf reference manual fails to build:

! Missing $ inserted.
<inserted text>
                $
l.532163 \bibitem[wp:Chow_ring]{wp:Chow_ring}
                                             {\phantomsection\label{sage/sch...

?
! Emergency stop.
<inserted text>
                $
l.532163 \bibitem[wp:Chow_ring]{wp:Chow_ring}
                                             {\phantomsection\label{sage/sch...

!  ==> Fatal error occurred, no output PDF file produced!

Changed 11 years ago by vbraun

Updated patch

comment:75 Changed 11 years ago by vbraun

  • Status changed from needs_work to positive_review

I removed the underscore in the offending reference name. The pdf developer reference builds now if #10721 (Increase LaTeX POOL_SIZE) is applied.

comment:76 Changed 11 years ago by jdemeyer

  • Merged in set to sage-4.6.2.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.