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: |
Description (last modified by )
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:
- trac_9713_fix_cardinality.patch
- trac_9713_fix_fg_pid.2.patch
- trac_9713_fix_fg_pid_reviewer.2.patch
- trac_9713_toric_chow_group.patch
- 9713_hash.patch
Depends on #8948.
Attachments (9)
Change History (85)
comment:1 Changed 12 years ago by
- Status changed from new to needs_info
comment:2 Changed 12 years ago by
- 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
- Status changed from needs_review to needs_work
Some preliminary comments:
- The new module is not completely documented yet,
sage -coverage
shows some issues. - It seems to me that it is more common to use A^{k} rather than A_{d-k}, so I think it would be better to stick with it in the documentation.
- I'd prefer to drop "The " from "The Chow cycle ..." in
_repr_
as was done for divisor classes. - It is not clear how to construct Chow cycles (except for those that correspond to cones).
- It is not clear what do numbers mean in the representation - i.e. how the basis is chosen?
- 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
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
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
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
- Status changed from needs_work to needs_review
Updated patch now applies cleanly. Ready for review...
comment:8 Changed 11 years ago by
- 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
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
- 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
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: ↓ 13 Changed 11 years ago by
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
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
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
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
- 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
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
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
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
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
- Status changed from needs_review to needs_work
- The discussed above fact that this patch implement the full Chow group should be reflected in module level documentation.
- Documentation from
ChowCycle.__init__
should be moved to the class docstring, otherwise it is invisible. - 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... - Line 170 misses the second ":" in the end.
- The first words of some docstrings have extra s'es, like "Returns" and "Intersects".
- 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? - For
intersect_with_divisor
it would be nice to give a reference to the used algorithm (Fulton p.97?). I think also thatintersection_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 perhapsi = I_gamma.pop()
does the same thing asi = 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? Chow_cycle
method of toric divisors does not have a doctest and input/output description.- 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
- It is probably worth mentioning what is the point of
A(Cone(cone))
afterA(cone)
. I actually didn't even know thatCone(cone)
works! Fortunately, it works as expected ;-) - 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 isn_{\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 orrelation_gens
?) and there should be a reference to where it was taken from. - 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? - 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. - 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." - I think it would be more natural if
gens
without parameters returned the union of allgens
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.) - For
relation_gens
wouldn't it be more natural to return elements of the covering module, i.e. lifts of the current output? - Line 979: you meant
== QQ
. - Would it make sense to derive
ChowGroup_degree_class
from some kind of a group rather thanSageObject
?
That's it for the current patch!
comment:23 Changed 11 years ago by
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
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
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 torelation_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
- Milestone changed from sage-4.6.1 to sage-4.6.2
- Status changed from needs_work to needs_review
comment:27 Changed 11 years ago by
- Status changed from needs_review to needs_work
Wait, I'm still meddling with the coercions...
comment:28 Changed 11 years ago by
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:
- Why
get_degree
and not justdegree
for Chow cycles? - 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
comment:29 follow-up: ↓ 30 Changed 11 years ago by
A6. Yes, integral "of". Will fix it ;-)
ChowGroup.degree()
is probably a stupid name... ideas for better names? But I wanted to distinguish it fromChowCycle.degree()
.
- There is no
ChowCycle.cohomology_class()
method nor any coercion to theCohomologyRing
.
comment:30 in reply to: ↑ 29 Changed 11 years ago by
Replying to vbraun:
ChowGroup.degree()
is probably a stupid name... ideas for better names? But I wanted to distinguish it fromChowCycle.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?
- There is no
ChowCycle.cohomology_class()
method nor any coercion to theCohomologyRing
.
What about line 444?..
comment:31 Changed 11 years ago by
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
- Status changed from needs_work to needs_review
comment:33 Changed 11 years ago by
- 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
comment:34 Changed 11 years ago by
- Status changed from needs_work to needs_review
comment:35 Changed 11 years ago by
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: ↓ 37 Changed 11 years ago by
- Description modified (diff)
comment:37 in reply to: ↑ 36 Changed 11 years ago by
comment:38 follow-ups: ↓ 39 ↓ 49 Changed 11 years ago by
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
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 thetrac_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: ↓ 41 Changed 11 years ago by
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: ↓ 43 Changed 11 years ago by
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: ↓ 45 Changed 11 years ago by
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
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
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
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: ↓ 47 Changed 11 years ago by
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
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
- 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 doesself.element_class
then the methodself.element_class()
is called, which computes a new class out ofself.Element
andself.category()
, assigns this new class to an actual (not lazy) attributeself.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
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
- 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 thetrac_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
- Work issues element class deleted
comment:51 Changed 11 years ago by
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
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
- 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
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
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
Ok I've rewritten it to be more careful with "degrees".
comment:57 Changed 11 years ago by
- Status changed from needs_info to needs_review
comment:59 Changed 11 years ago by
- Description modified (diff)
comment:60 Changed 11 years ago by
- Status changed from positive_review to needs_work
- Work issues set to rebase
Needs to be rebased to sage-4.6.2.alpha2
comment:61 Changed 11 years ago by
- 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
What was necessary in rebasing??? I still have my queue applied smoothly on alpha2!
comment:63 Changed 11 years ago by
- 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
- Description modified (diff)
comment:65 Changed 11 years ago by
- 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?
comment:66 follow-up: ↓ 68 Changed 11 years ago by
- Description modified (diff)
Rebased on top of Sage-4.6.2.alpha2+#8948.
comment:67 Changed 11 years ago by
- Status changed from needs_work to positive_review
comment:68 in reply to: ↑ 66 Changed 11 years ago by
comment:69 Changed 11 years ago by
- 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
- 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
- Status changed from new to needs_work
Changed 11 years ago by
comment:72 Changed 11 years ago by
- 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
- Status changed from needs_review to positive_review
Thanks for catching this!
comment:74 Changed 11 years ago by
- 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!
comment:75 Changed 11 years ago by
- 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
- Merged in set to sage-4.6.2.alpha4
- Resolution set to fixed
- Status changed from positive_review to closed
Volker, is this one ready for review? (It needs a little rebasement on top of new #9337.)