Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use weakref for PolyBoRi #3284

Closed
malb opened this issue May 23, 2008 · 14 comments
Closed

use weakref for PolyBoRi #3284

malb opened this issue May 23, 2008 · 14 comments

Comments

@malb
Copy link
Member

malb commented May 23, 2008

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.

CC: @burcin @sagetrac-PolyBoRi

Component: commutative algebra

Keywords: PolyBoRi, editor_malb

Issue created by migration from https://trac.sagemath.org/ticket/3284

@malb malb added this to the sage-3.0.2 milestone May 23, 2008
@malb malb self-assigned this May 23, 2008
@sagetrac-mabshoff sagetrac-mabshoff mannequin modified the milestones: sage-3.0.2, sage-3.0.3 May 23, 2008
@sagetrac-PolyBoRi
Copy link
Mannequin

sagetrac-PolyBoRi mannequin commented May 23, 2008

comment:2

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

@malb
Copy link
Member Author

malb commented May 25, 2008

comment:3

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

@malb
Copy link
Member Author

malb commented May 25, 2008

Attachment: pbori_weakref.patch.gz

this patch is supposed to work

@malb

This comment has been minimized.

@malb
Copy link
Member Author

malb commented May 25, 2008

Changed keywords from PolyBoRi, segfault to PolyBoRi

@malb malb changed the title [with patch, needs work, SEGFAULTs!] use weakref for PolyBoRi use weakref for PolyBoRi May 25, 2008
@malb
Copy link
Member Author

malb commented May 25, 2008

comment:5

Please review the updated patch which fixes the doctest failure.

@burcin
Copy link

burcin commented Jun 4, 2008

comment:6

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.

@malb
Copy link
Member Author

malb commented Jun 4, 2008

comment:7

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.

@craigcitro
Copy link
Member

Changed keywords from PolyBoRi to PolyBoRi, editor_malb

@craigcitro craigcitro changed the title use weakref for PolyBoRi [with patch, under review by burcin before 6/20] use weakref for PolyBoRi Jun 15, 2008
@burcin
Copy link

burcin commented Jun 20, 2008

BooleanPolynomialRing user friendly names

@burcin
Copy link

burcin commented Jun 20, 2008

comment:9

Attachment: trac3284_BooleanPolynomialRing_normalize_names.patch.gz

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.

@burcin burcin changed the title [with patch, under review by burcin before 6/20] use weakref for PolyBoRi use weakref for PolyBoRi Jun 20, 2008
@malb
Copy link
Member Author

malb commented Jun 24, 2008

comment:10

Burcin's patch looks good and passes doctests.

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Jun 25, 2008

comment:11

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

Cheers,

Michael

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Jun 25, 2008

comment:12

Merged in Sage 3.0.4.alpha1

@sagetrac-mabshoff sagetrac-mabshoff mannequin modified the milestones: sage-3.1.1, sage-3.0.4 Jun 25, 2008
@sagetrac-mabshoff sagetrac-mabshoff mannequin closed this as completed Jun 25, 2008
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants