document behavior of .is_prime() for number fields
Closed this issue · 11 comments
This patch addresses the issue raised in #7596 comment:15.
(I am creating a new ticket for this because #7596 is about something much more general.)
CC: @nbruin
Component: algebra
Keywords: prime elements
Author: Lorenz Panny
Branch/Commit: 5c0a348
Reviewer: Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/32340
Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.
Description changed:
---
+++
@@ -1,3 +1,3 @@
-This patch addresses the issue raised in comment 15 of #7596.
+This patch addresses the issue raised in [#7596 comment:15](https://github.com/sagemath/sage/issues/7596#comment:15).
(I am creating a new ticket for this because #7596 is about something much more general.)I am suspicious that this is the correct place to put this documentation since the method just is calling the super one. That suggests it should go in the implementation of the base class.
I did it like this because #7596 comment:15 said:
In this case, it probably means equipping number field elements with a fresh implementation of
is_primewith appropriate doc.
Presumably, the way the patch is currently done increases the visibility of the warning to those users who need to see it: They'd most likely be reading the number fields documentation, not the rings documentation. But I don't have a strong opinion either way.
Replying to @yyyyx4:
I did it like this because #7596 comment:15 said:
In this case, it probably means equipping number field elements with a fresh implementation of
is_primewith appropriate doc.Presumably, the way the patch is currently done increases the visibility of the warning to those users who need to see it: They'd most likely be reading the number fields documentation, not the rings documentation. But I don't have a strong opinion either way.
I don’t quite buy that argument. From the interactive documentation with ?, you see it all the same. I don’t think it makes that much difference in the online doc. It does add an extra (Python) indirection, which means the code it (albeit likely just marginally) slower. The same message would, in principle, apply for anything using the base class as well.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5c0a348 | document that number fields implement is_prime() incompatibly |
Alright, how about this?
Reviewer: Travis Scrimshaw
Thank you. I think that is a much more natural place to put it (which reflects the fact that it uses the ideal() method).
Perhaps interesting, we get oddities like this:
sage: z = QQ['z'].0
sage: K = NumberField(z - 1,'s'); K
Number Field in s with defining polynomial z - 1
sage: K(7).is_prime()
True
I guess that is what structure you want to put on it: a number field or a usual field. Perhaps this is one way out of #7596, to have QQ.number_field() return the object above.
Changed branch from public/document_is_prime_behavior_in_number_fields to 5c0a348