Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#13145 closed defect (fixed)

Sage's noncommutative rings don't always increment a refcount

Reported by: roed Owned by: malb
Priority: critical Milestone: sage-5.4
Component: commutative algebra Keywords:
Cc: SimonKing, malb Merged in: sage-5.4.beta1
Authors: David Roe Reviewers: Martin Albrecht
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by roed)

Running doctests with the new doctest framework revealed that KeyErrors were being ignored in sage.lib.singular.ring.singular_ring_delete.

Attachments (1)

13145.patch​ (3.1 KB) - added by roed 7 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by roed

  • Description modified (diff)

Changed 7 years ago by roed

comment:2 Changed 7 years ago by roed

  • Status changed from new to needs_review

comment:3 Changed 7 years ago by roed

  • Status changed from needs_review to needs_work

My fixes apparently aren't enough: I'm picking up more failures now.

comment:4 Changed 7 years ago by roed

  • Description modified (diff)

comment:5 Changed 7 years ago by kini

  • Summary changed from Sage's noncommutive rings don't always increment a refcount to Sage's noncommutative rings don't always increment a refcount

comment:6 Changed 7 years ago by roed

  • Status changed from needs_work to needs_review

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.

comment:7 Changed 7 years ago by jhpalmieri

  • Cc SimonKing added

Simon: any opinions?

comment:8 Changed 7 years ago by roed

  • Cc malb added

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.

comment:9 Changed 7 years ago by malb

  • Reviewers set to Martin Albrecht
  • Status changed from needs_review to positive_review

Patch looks good to me.

comment:10 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.3 to sage-5.4

comment:11 Changed 7 years ago by jdemeyer

  • Priority changed from minor to critical

Thanks!

comment:12 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.4.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 Changed 7 years ago by SimonKing

Question: Wouldn't it be better to follow the approach in #13447? Hence, drop the ring_refcount_dict altogether?

comment:14 Changed 7 years ago by SimonKing

For the record: Since this ticket is already merged, I made it an indirect dependency of #13447, which is now ready for review. #13447 removes the ring_refcount_dict.

Note: See TracTickets for help on using tickets.