Add Chow package for intersection theory #40023
Conversation
is_Integer and is_MPolynomialRing deprecated, replaced them with isinstance(..., TYPE)
|
new files must be added to the meson.build files in their directory |
fix rst syntax in doc there
|
I allowed myself to make a direct commit on the branch, so please pull before working further. |
|
Documentation preview for this PR (built with commit 23c9d4e; changes) is ready! 🎉 |
There was a problem hiding this comment.
Thank you for adding this. This will be a great addition to have. Right now, from a quick look I have some general comments/questions:
- You should not import many of the things into the global namespace (in particular, those in
chow/all.py). This could create some conflicts with things already in Sage and pollutes the namespace. There should be better ways to access these objects. See below for some suggestions. - I don't quite see a reason for having a
ChowSchemeclass. I understand the basic principle, that it is for schemes that know their Chow ring. However, I think this is best directly added into theSchemeclass (and related constructions) that call thechow_ring()method. For the schemes where this is known. You would still have the specific subclasses. The input to scheme constructions perhaps could then be extended to accept the Chow ring data. - The readme file is better as the thematic tutorial or the top-level documentation of one of the chow/* files (preferably some "main" or highly-likely-to-be-accessed file).
- For the schemes where the Chow ring is known, I would create a "catalog" like how we have for, e.g., algebras.
- It would be good if this could fit in with the toric varieties Chow group (I believe they are related, but I am not an expert): https://doc.sagemath.org/html/en/reference/schemes/sage/schemes/toric/chow_group.html
- General
Returns->ReturnandEXAMPLE::->EXAMPLES::following our doc conventions.
| sage: x.truncate(1, 2) | ||
| 4*E^2 + 3*H - E | ||
| """ | ||
| return sum([x for x in self.by_degrees()[a:b + 1]]) |
There was a problem hiding this comment.
| return sum([x for x in self.by_degrees()[a:b + 1]]) | |
| return self.parent().sum(x for x in self.by_degrees()[a:b + 1]) |
In case the result is 0 in order to have the correct parent.
| sage: x = 2 + 3*h - 5*h^3 | ||
| sage: TestSuite(x).run() | ||
| """ | ||
| self._parentchowring = parent |
There was a problem hiding this comment.
You don't need to set this attribute since it is accessible via self.parent().
| """ | ||
| A = self._parentchowring | ||
| if A.ngens() == 0: | ||
| return richcmp(QQ(str(self)), QQ(str(other)), op) |
There was a problem hiding this comment.
Why does this need a special case? Also, going via strings seems like a very slow way to do it.
| - ``a`` -- an optional integer (defaults to 0) | ||
|
|
||
| - ``b`` -- an optional integer (defaults to 0) |
There was a problem hiding this comment.
| - ``a`` -- an optional integer (defaults to 0) | |
| - ``b`` -- an optional integer (defaults to 0) | |
| - ``a`` -- (default: 0) integer | |
| - ``b`` -- (default: 0) integer |
| return self.lift().degree() | ||
|
|
||
| @cached_method | ||
| def _expp(self): |
There was a problem hiding this comment.
I don't understand these internal method docstrings. Please rewrite.
|
Thank you both for the useful comments and suggestions! I am not the author of this code, so I can’t comment on design choices made by the authors several years ago; |
|
You still need to add two new "meson.build" files, one inside chow/ and one inside library (I think). |
|
Indeed, but I think we should make sure it is done in a good way before we integrate it in. I can help, but I don’t know how much time I will have in the next few weeks to do a significant amount on this. I can definitely discuss things on here if you have any questions. |
|
I allowed myself again to make a direct commit on the branch, so please pull before working further. |
|
Dear all, Thank you both again. I have thought about your remarks, and I am implementing most of your suggestions. However, I believe there is a design choice to be made to properly address your remark 2 @tscrim (and consequently remark 5): see below. The issue is that schemes do not, in general, admit a Chow group; only varieties, which are a special case of schemes, do. This means that for mathematical consistency, one would need a Introducing the ChowRing as an attribute of I would like to ask for advice on such a design choice, since I think it would go beyond the scope of this library alone… |
|
probably you should remove the empty |
|
@tobiasdiez : what needs to be done here about the meson.build files so that it works, please ? |
|
Judging from the log, it should be enough to simply add the autogenerated |
|
I would go for the first option : please add one line to the file |
|
Thank you for the help, now it builds (but still fails a test in the thematic tutorial, will fix that). The necessary design choice of |
📝 Checklist
Description
This PR merges the Chow library, written by Christoph Sorger (@sorger-c) and Manfred Lehn, into Sagemath.
The library is otherwise only accessible by checking out a specific commit of Sage, which is not ideal for users who are not git-savy.
This completes the integration discussed in #27228 by @sorger-c and @fchapoton.