Laurent polynomials, Fitting ideals and characteristic varieties #36368
Conversation
enriqueartal
changed the title
| g = f.__reduce__()[1][0] | ||
| if self.ring().ngens() > 1: | ||
| g = f.__reduce__()[1][0] | ||
| else: | ||
| g = f.__reduce__()[1][1] |
There was a problem hiding this comment.
Those doctests don't really test why you made this change. Something was broken: the containment check for univariate Laurent polynomials. As such, you need to add that test in showing the contains now works properly in that case.
| @coerce_binop | ||
| def divides(self, other): | ||
| """ | ||
| Check if ``self`` divides ``other`` |
There was a problem hiding this comment.
| Check if ``self`` divides ``other`` | |
| Check if ``self`` divides ``other``. |
There was a problem hiding this comment.
For the comment about the doctests, I am not sure what you mean, check the new tests please. In particular, I saw something I do not like really but I am not sure if it should be changed; the ring LaurentPolynomialRing(QQ) is treated as univariate, while the ring LaurentPolynomialRing(QQ, 1) not. I know that @miguelmarco has looked at the code of Laurent polynomials and saw more things to be improved. Maybe @kedlaya could help and explain about those differences and also about the goal of hint.
For the other one, changed.
For the slowness, I really appreciate your patience and the actual fact that the code is being improved thanks to you.
Besides these changes, I think the big issue is about change_ring.
There was a problem hiding this comment.
I have given you the explicit doctests for ideal.py I want, but if you look at the code in your doctests, it is indirectly calling your new method. As such, if someone changes the change_ring for Laurent polynomial ideals (such as you first did), then your new method doesn't get tested. Which means bugs could be introduced. Good practice is you should directly test the method in question (to the extent it is feasible).
For the __contains__ doctests, please remove (and apply fire and holy water) to any doctest that explicitly calls __reduce__. That is not testing that method at all. Better tests are also x in I rather than I.__contains__(x) as the latter is more natural for Python. In particular the line 196 and 205 tests are redundant. The test I wanted added was the one in line 203 since that did not work before your change (well, I didn't check, but basing this on the code change).
There was a problem hiding this comment.
For the comment about the doctests, I am not sure what you mean, check the new tests please. In particular, I saw something I do not like really but I am not sure if it should be changed; the ring
LaurentPolynomialRing(QQ)is treated as univariate, while the ringLaurentPolynomialRing(QQ, 1)not. I know that @miguelmarco has looked at the code of Laurent polynomials and saw more things to be improved. Maybe @kedlaya could help and explain about those differences and also about the goal ofhint.
The purpose of hint is to speed up computing the Groebner basis of the polynomial ideal in some internal constructions where one has some prior information about the result (e.g., taking the join of two Laurent polynomial rings).
There was a problem hiding this comment.
The purpose of
hintis to speed up computing the Groebner basis of the polynomial ideal in some internal constructions where one has some prior information about the result (e.g., taking the join of two Laurent polynomial rings).
Wouldn't it be better to initialize with the ideal generated by (the corresponding polynomials of) the generators? If I understand it correctly, it would satisfy the needed condition, and would be a better bound.
There was a problem hiding this comment.
I have changed the tests in ideal.py and also in __contains__, I hope I have understood @tscrim. I changed the code since it was not a good idea to check the number of generators, replaced by an isInstance. To avoid possible issues in toric_coordinate_change, __reduce__ has been replaced by monomial_reduction which is independent of the particular instance of Laurent polynomials.
For hint, I wonder if in its first definition, it would be better to take the monomial reduction of the generators; it should not be expensive and maybe it is more clear. It will be far from a Groebner basis in general, but at least, they generate a closer ideal.
Thanks for your help
|
In order to be more consistent (I think) now the second term of |
|
I made the corrections. Thanks, Enrique |
vbraun
force-pushed
the
develop
branch
2 times, most recently
from
883e05f
to
e349b00
Compare
|
Documentation preview for this PR (built with commit e14d69a; changes) is ready! 🎉 |
…tic varieties
<!-- Describe your changes here in detail -->
Recently in sagemath#36128 (already in develop) characteristic varieties of
finitely presented fundamental groups were introduced. Its computation
is based on Fitting ideals of Laurent polynomial matrices. In sagemath#36299,
Fitting ideals were implemented for generic rings with some improvements
for PID and polynomial rings.
There are two original goals in this PR: to improve the output of
characteristic varieties and to use the cited implementation. In order
to make computations faster, the implementation of Fitting ideals should
apply to Laurent polynomial rings and for this goal, several changes
should be applied to Laurent polynomials in `Sagemath`.
I am not sure if a deeper change should be made, since I applied only
the changes I needed for the above goal:
- src/sage/groups/finitely_presented.py:
- Changes in `fitting_ideals`.
- src/sage/matrix/matrix2.py: Style changes.
- src/sage/matrix/matrix_laurent_mpolynomial_dense.pyx: This is a new
file to create the class `Matrix_laurent_mpolynomial_dense`.
- A method `laurent_matrix_reduction` to obtain a matrix of
polynomials where the variables are non common factors for neither the
rows nor the columns.
- A methord `_fitting_ideal` to use the same method for matrices of
polynomials.
- src/sage/matrix/matrix__mpolynomial_dense.pyx: Style changes.
The main changes are for Laurent polynomials to avoid errors in the
above implementations.
- src/sage/rings/polynomials/laurent_polynomial.pyx:
- Style changes.
- Create `xgcd` needed for `inverse_mod`.
- Create `inverse_mod`.
- Create `divides`, I copied the code for polynomials with minor
changes.
- src/sage/rings/polynomials/laurent_polynomial_ideal.py:
- Style changes.
- Changes in hint keyword in `__init__`, the previous code create
issues, e.g., impossible to sum ideals of univariate Laurent polynomial
rings. They involve changes in doctests for `hint`
- Changes in `__contains__` since `__reduce__` is different for
univariate and multivaraite case.
- Create `gens_reduced`.
- Changes in `polynomial_ideal` to deal differently if uni- and
multi-variate.
- src/sage/rings/polynomials/laurent_polynomial_mpair.py:
- Style changes.
- Create `divides`, I copied the code for polynomials with minor
changes.
- src/sage/rings/polynomials/laurent_polynomial_ring.py:
- Style changes.
- Some `TestSuite`'s applied to domains as base_rings; the
corresponding `TestSuite`'s for polynomials also failed if applied to
polynomial rings.
- src/sage/rings/polynomials/laurent_polynomial_ring_base.py:
- Style changes.
- Implement `is_noetherian`.
- src/sage/rings/polynomials/polynomial_element.pyx: Style changes.
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->
- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36368
Reported by: Enrique Manuel Artal Bartolo
Reviewer(s): Enrique Manuel Artal Bartolo, kedlaya, miguelmarco, Travis Scrimshaw
|
The |
|
Mathematically it is fine (because it is determined up to a unit), but programmatically we want gcd and xgcd to match. See #17671. |
Recently in #36128 (already in develop) characteristic varieties of finitely presented fundamental groups were introduced. Its computation is based on Fitting ideals of Laurent polynomial matrices. In #36299, Fitting ideals were implemented for generic rings with some improvements for PID and polynomial rings.
There are two original goals in this PR: to improve the output of characteristic varieties and to use the cited implementation. In order to make computations faster, the implementation of Fitting ideals should apply to Laurent polynomial rings and for this goal, several changes should be applied to Laurent polynomials in
Sagemath.I am not sure if a deeper change should be made, since I applied only the changes I needed for the above goal:
fitting_ideals.Matrix_laurent_mpolynomial_dense.laurent_matrix_reductionto obtain a matrix of polynomials where the variables are non common factors for neither the rows nor the columns._fitting_idealto use the same method for matrices of polynomials.The main changes are for Laurent polynomials to avoid errors in the above implementations.
xgcdneeded forinverse_mod.inverse_mod.divides, I copied the code for polynomials with minor changes.__init__, the previous code create issues, e.g., impossible to sum ideals of univariate Laurent polynomial rings. They involve changes in doctests forhint__contains__since__reduce__is different for univariate and multivaraite case.gens_reduced.polynomial_idealto deal differently if uni- and multi-variate.divides, I copied the code for polynomials with minor changes.TestSuite's applied to domains as base_rings; the correspondingTestSuite's for polynomials also failed if applied to polynomial rings.is_noetherian.📝 Checklist
⌛ Dependencies