Opened 7 years ago

Closed 6 years ago

#3284 closed defect (fixed)

[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 Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by malb)

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 (2)

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

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by mabshoff

  • Milestone changed from sage-3.0.2 to sage-3.0.3

comment:2 Changed 7 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 7 years ago by malb

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

Changed 7 years ago by malb

this patch is supposed to work

comment:4 Changed 7 years ago by malb

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

comment:5 Changed 7 years ago by malb

Please review the updated patch which fixes the doctest failure.

comment:6 Changed 6 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 6 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 6 years ago by craigcitro

  • Keywords editor_malb added
  • 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 6 years ago by burcin

BooleanPolynomialRing? user friendly names

comment:9 Changed 6 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 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 6 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 6 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 6 years ago by mabshoff

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

Merged in Sage 3.0.4.alpha1

Note: See TracTickets for help on using tickets.