Opened 10 years ago

Closed 10 years ago

Last modified 10 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:

Status badges

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 10 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 10 years ago by roed

  • Description modified (diff)

Changed 10 years ago by roed

comment:2 Changed 10 years ago by roed

  • Status changed from new to needs_review

comment:3 Changed 10 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 10 years ago by roed

  • Description modified (diff)

comment:5 Changed 10 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 10 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 10 years ago by jhpalmieri

  • Cc SimonKing added

Simon: any opinions?

comment:8 Changed 10 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 10 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 10 years ago by jdemeyer

  • Milestone changed from sage-5.3 to sage-5.4

comment:11 Changed 10 years ago by jdemeyer

  • Priority changed from minor to critical

Thanks!

comment:12 Changed 10 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 10 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 10 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.