Handle various extension degrees in pAdicGenericElement initialisation #39583
Conversation
|
Documentation preview for this PR (built with commit 9694163; changes) is ready! 🎉 |
|
Turns out I was not the first one to notice that the bug was equivalent to Fixes #33527. |
There was a problem hiding this comment.
Thanks a lot for helping out with p-adics in SageMath :)
I don't understand yet what you are trying to achieve in your implementation.
I am also a bit worried that minimal_polynomial() can be quite expensive. It seems that you are mostly interested whether an element is a base element, i.e., whether x.polynomial().degree() <= 0?
Wouldn't it be enough to do this to break the recursion that this PR is all about?
-else:
+elif self.base_ring() is not self:|
If you're interested in p-adics and local things in SageMath, there's a channel on zulip where we tend to hang out: https://sagemath.zulipchat.com/#narrow/channel/271072-padics Note that we're also meeting this Friday to work on valuations (and p-adics) feel free to join: https://calendar.app.google/3vhFJPDvuWhtSyB6A |
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
|
Looks good to me. |
sagemathgh-39583: Handle various extension degrees in pAdicGenericElement initialisation I have looked into bug sagemath#28555. I have been able to reproduce it with version 10.6.beta7. It turns out the infinite loop can be reproduced with: ``` sage: A.<a> = Qq(5^2) sage: A.base_ring()(a) ``` But the following also triggers the bug: `A.base_ring()(A(1))`. This is bug sagemath#33527. The culprit is in the file `src/sage/rings/padics/padic_template_element.pxi`. Calling `A.base_ring()(a)` will call `pAdicTemplateElement.__init__` which will call `A.base_ring()(a)` again on line 140. To fix it, I added a check for when the degree of the polynomial (over the base ring $\mathbb Z_p$ or $\mathbb Q_p$) defining the element is greater than one. If we were trying to convert to the base ring, I raise a `TypeError` exception. Else, I raise a `NotImplementedError` exception (this is the case when one tries to convert between two extensions, it should be implemented later, but here I only fix the bug). Finally, I change line 140 from `x = self.base_ring()(x)` to `x = self.base_ring()(x.polynomial().constant_coefficient())` to avoid the infinite loop. NOTE: this is my "hello world" PR, I may have broken something with this, please be kind. Please do not hesitate to tell me how to do better. Fixes sagemath#28555. Fixes sagemath#33527. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> None URL: sagemath#39583 Reported by: Rubén Muñoz--Bertrand Reviewer(s): Julian Rüth, roed314, Rubén Muñoz--Bertrand
I have looked into bug #28555. I have been able to reproduce it with version 10.6.beta7.
It turns out the infinite loop can be reproduced with:
But the following also triggers the bug:
A.base_ring()(A(1)). This is bug #33527.The culprit is in the file
src/sage/rings/padics/padic_template_element.pxi. CallingA.base_ring()(a)will callpAdicTemplateElement.__init__which will callA.base_ring()(a)again on line 140.To fix it, I added a check for when the degree of the polynomial (over the base ring$\mathbb Z_p$ or $\mathbb Q_p$ ) defining the element is greater than one. If we were trying to convert to the base ring, I raise a
TypeErrorexception. Else, I raise aNotImplementedErrorexception (this is the case when one tries to convert between two extensions, it should be implemented later, but here I only fix the bug).Finally, I change line 140 from
x = self.base_ring()(x)tox = self.base_ring()(x.polynomial().constant_coefficient())to avoid the infinite loop.NOTE: this is my "hello world" PR, I may have broken something with this, please be kind. Please do not hesitate to tell me how to do better.
Fixes #28555.
Fixes #33527.
📝 Checklist
⌛ Dependencies
None