Opened 11 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:

GitHub link to the corresponding issue

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

Download all attachments as: .zip

Change History (15)

comment:1 Changed 11 years ago by roed

Description: modified (diff)

Changed 11 years ago by roed

Attachment: 13145.patch​ added

comment:2 Changed 11 years ago by roed

Status: new → needs_review

comment:3 Changed 11 years ago by roed

Status: needs_review → needs_work

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

comment:4 Changed 11 years ago by roed

Description: modified (diff)

comment:5 Changed 11 years ago by kini

Summary: Sage's noncommutive rings don't always increment a refcount → Sage's noncommutative rings don't always increment a refcount

comment:6 Changed 11 years ago by roed

Status: needs_work → 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: → Martin Albrecht
Status: needs_review → positive_review

Patch looks good to me.

comment:10 Changed 10 years ago by jdemeyer

Milestone: sage-5.3 → sage-5.4

comment:11 Changed 10 years ago by jdemeyer

Priority: minor → critical

Thanks!

comment:12 Changed 10 years ago by jdemeyer

Merged in: → sage-5.4.beta1
Resolution: → fixed
Status: positive_review → 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.