Ticket #3284 (closed defect: fixed)

Opened 5 years ago

Last modified 5 years ago

[with patch, positive review] use weakref for PolyBoRi

Reported by: malb Owned by: malb
Priority: major Milestone: sage-3.0.4
Component: commutative algebra Keywords: PolyBoRi, editor_malb
Cc: burcin, PolyBoRi Work issues:
Report Upstream: Reviewers:
Authors: Merged in:
Dependencies: Stopgaps:

Description (last modified by malb) (diff)

This patch makes sure only one BooleanPolynomialRing per parameter set is created by returning a reference to a prior created object if there is such a reference.

Attachments

pbori_weakref.patch Download (6.1 KB) - added by malb 5 years ago.
this patch is supposed to work
trac3284_BooleanPolynomialRing_normalize_names.patch Download (3.0 KB) - added by burcin 5 years ago.
BooleanPolynomialRing? user friendly names

Change History

comment:1 Changed 5 years ago by mabshoff

  • Milestone changed from sage-3.0.2 to sage-3.0.3

comment:2 Changed 5 years ago by PolyBoRi

Hello again, "Operands come from different manager." is an error message from the deep of the BDD-package behind the PolyBoRi? data structures. It means, that one tries to do a binary operation with elements from two different rings. But operation does not necessary mean the y*z operation, it maybe triggered by something else in N() oder _coerce_

Best regards,

Alexander

comment:3 Changed 5 years ago by malb

My guess is that we somewhere forget to set the global ring maybe?

Changed 5 years ago by malb

this patch is supposed to work

comment:4 Changed 5 years ago by malb

  • Keywords PolyBoRi added; PolyBoRi, segfault removed
  • Description modified (diff)
  • Summary changed from [with patch, needs work, SEGFAULTs!] use weakref for PolyBoRi to [with patch, needs review] use weakref for PolyBoRi

comment:5 Changed 5 years ago by malb

Please review the updated patch which fixes the doctest failure.

comment:6 Changed 5 years ago by burcin

I don't see why the changes to pbori.pyx other than the addition of the R._pbring.activate() at line 4131 are necessary.

In BooleanPolynomialRing_constructor, the if statement (including the elif) at line 430 seems to be redundant, since normalize_names already returns a tuple. It could be that I'm reading the source of normalize_names wrong though.

comment:7 Changed 5 years ago by malb

True, it is unrelated to this particular weakref patch. I mixed up two things. The renaming only cleans up since vars is a built-in identifier and it is considered bad practice to use it like we used to use it.

comment:8 Changed 5 years ago by craigcitro

  • Keywords PolyBoRi, editor_malb added; PolyBoRi removed
  • Summary changed from [with patch, needs review] use weakref for PolyBoRi to [with patch, under review by burcin before 6/20] use weakref for PolyBoRi

Changed 5 years ago by burcin

BooleanPolynomialRing? user friendly names

comment:9 Changed 5 years ago by burcin

  • Summary changed from [with patch, under review by burcin before 6/20] use weakref for PolyBoRi to [with patch, needs review] use weakref for PolyBoRi

BooleanPolynomialRing_constructor in malb's original patch fails when only names are provided, attachment:trac3284_BooleanPolynomialRing_normalize_names.patch Download fixes this case.

I give malb's patch (followed by mine) a positive review. Someone should review my patch, especially the change to normalize_names.

comment:10 Changed 5 years ago by malb

  • Summary changed from [with patch, needs review] use weakref for PolyBoRi to [with patch, positive review] use weakref for PolyBoRi

Burcin's patch looks good and passes doctests.

comment:11 Changed 5 years ago by mabshoff

All doctests pass with the patch applied and valgrind gives pbori.pyx a clean bill of health.

Cheers,

Michael

comment:12 Changed 5 years ago by mabshoff

  • Status changed from new to closed
  • Resolution set to fixed
  • Milestone changed from sage-3.1.1 to sage-3.0.4

Merged in Sage 3.0.4.alpha1

Note: See TracTickets for help on using tickets.