New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bug in divides #5759
Comments
This comment has been minimized.
This comment has been minimized.
comment:1
Attachment: divides.patch.gz |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comment:7
Attachment: 5759-doctests.patch.gz |
comment:8
This breaks a doctest somewhere else:
|
comment:9
The whole divides function is broken (nearly irreparable) with the effect that "divides" failes or gives wrong answers if and only if the ring is no polynomial ring or ZZ (which have their own (buggy) implementation). The reason is that not all commutative rings are euclidean domains and "modulo" operation does not do what it is supposed to do in general commutative rings, for example in finite integer rings: Zmod(12)(3) % Zmod(12)(4) gives Zmod(3)(0) (which is also broken, by the way). The only correct behaviour would be:
Also: all doctests in element.py and other files are for polynomial rings only, with the effect that many functions in SAGE fail or give wrong answers if the ring is no polynomial ring. Greetings, |
comment:11
I would have thought that even in this generality (element of a commutative ring) it would be worth adding the following special cases:
where in each case the test is wrapped in a try/except block so that if (for example) self.is_unit() is not implemented then it just passes. Finally, if none of the above works then default to code as it is now. Any individual ring where the x%self computation will not work but where there is an easy alternative direct test (such as in Integers(n)) can have their own specific implementations. If there is any support for this I'm willing to try to do it. |
comment:12
Note that #5347 (merged in 4.1.2.alpha2) fixes the easy cases I listed above. We still have
but this looks fine to me:
Hence the patches here need to be rebased and simplified to cater for the first one. |
comment:13
In fact the original patches can be discarded since they only fixed things that have been fixed anyway in #5347. What we do not have is a check the self and other are in "the same ring", which is not so obvious -- the simplest would be to throw an error if the parents were not identical, but that seems to harsh (it would cause 1.divides(1/2) to fail). Better would be to coerce into a common parent first -- the sort of thing which is done for |
comment:15
I add a patch to solve this problem. This patch is applied alone. Previous patches are discarded due to changes in the function during these months. See #5347 The algorithm first checks if parents coincide. If so, it runs the existing code. In other case, coerces both element to a common parent using the coercion model an runs the existing code on the coerced elements. The patch works on 4.5.3 |
Attachment: trac-5759.patch.gz |
comment:16
The patch applies fine to 4.6.alpha1, and all test pass (I tested the whole sage library on account of earlier difficulties). No generic function can deliver everything, but this deals with simple generic cases, as the new doctests show. |
Author: Luis Felipe Tabera |
Reviewer: John Cremona |
Merged: sage-4.6.alpha3 |
The function "divides" does not work for generic commutative rings.
Zmod(5)(1).divides(Zmod(2)(1)) is "True"
No division by zero checking is done! This gives for example an error if you type
-> Zmod(2).zero_ideal() == Zmod(2).zero_ideal()
-> Zmod(2).zero_ideal() == Zmod(2).unit_ideal()
This patch should fix this. It may not be able to handle all cases but classes who need a more clever function should do their own implementation anyways.
Greetings,
Kilian.
Component: basic arithmetic
Author: Luis Felipe Tabera
Reviewer: John Cremona
Merged: sage-4.6.alpha3
Issue created by migration from https://trac.sagemath.org/ticket/5759
The text was updated successfully, but these errors were encountered: