Sage's noncommutative rings don't always increment a refcount
roed314 opened this issue · 13 comments
Running doctests with the new doctest framework revealed that KeyErrors were being ignored in sage.lib.singular.ring.singular_ring_delete.
Component: commutative algebra
Author: David Roe
Reviewer: Martin Albrecht
Merged: sage-5.4.beta1
Issue created by migration from https://trac.sagemath.org/ticket/13145
Description changed:
---
+++
@@ -1 +1 @@
-Running doctests with the new doctest framework revealed that KeyErrors were being ignored in `sage.lib.singular.ring.singular_ring_delete`. The included patch fixes the problem, but I need to add doctests.
+Running doctests with the new doctest framework revealed that `KeyErrors` were being ignored in `sage.lib.singular.ring.singular_ring_delete`. The included patch fixes the problem, but I need to add doctests.Attachment: 13145.patch.gz
My fixes apparently aren't enough: I'm picking up more failures now.
Description changed:
---
+++
@@ -1 +1 @@
-Running doctests with the new doctest framework revealed that `KeyErrors` were being ignored in `sage.lib.singular.ring.singular_ring_delete`. The included patch fixes the problem, but I need to add doctests.
+Running doctests with the new doctest framework revealed that `KeyErrors` were being ignored in `sage.lib.singular.ring.singular_ring_delete`.Somehow the problems went away with further changes in the doctest framework. I'm not convinced that no problems remain in this area, but I'm going to return this to "Needs review" for now.
Simon: any opinions?
I'm going to make another push to finish #12415. If this could get reviewed sometime in the next week that would be great. I'm not sure if the changes here address all of the issues in deallocating singular objects, but I think it's an improvement and can go in.
Patch looks good to me.
Reviewer: Martin Albrecht
Thanks!
Merged: sage-5.4.beta1
Question: Wouldn't it be better to follow the approach in #13447? Hence, drop the ring_refcount_dict altogether?