Opened 6 years ago
Closed 6 years ago
#1541 closed enhancement (fixed)
[with patch, postive review] improve PolyBoRi integration
Reported by: | malb | Owned by: | malb |
---|---|---|---|
Priority: | major | Milestone: | sage-2.10 |
Component: | commutative algebra | Keywords: | |
Cc: | burcin | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
The attached patch provides:
- mq.SR can now construct PolyBoRi? polynomial systems
- some comments added to pbori.pyx
- BooleanPolynomial.__hash__
- BooleanPolynomial.variables
- coercion of GF(2) elements to BooleanPolynomialRing
- BooleanPolynomialRing.__call__ accepts strings
- _sig_on/_sig_off around groebner_basis call
Attachments (1)
Change History (12)
comment:1 Changed 6 years ago by rlm
- Summary changed from [with patch] improve PolyBoRi integration to [with patch, negative review] improve PolyBoRi integration
comment:2 Changed 6 years ago by rlm
results of ./sage -t -valgrind devel/sage-main/sage/crypto/mq/sr.py are at http://sage.math.washington.edu/home/rlmill/.sage/sage-memcheck.850
comment:3 Changed 6 years ago by rlm
Disregard those valgrind results- they are useless.
comment:4 Changed 6 years ago by malb
- Summary changed from [with patch, negative review] improve PolyBoRi integration to [with patch] improve PolyBoRi integration
I think the negative review has been resolved, and the currRing patch attached to this ticket should be applied (given a positive review) soon.
comment:5 Changed 6 years ago by burcin
Following Martin's comments on the slowdown caused by using __getattr__ in BooleanPolynomial, attached patch with file name polybori_booleanpolynomial_getattr.patch makes the methods directly available.
Some timings (using random polynomials in each case might not be a good idea, but it still demonstrates the point):
Without the patch:
sage: P = BooleanPolynomialRing(100,'x') sage: from polybori.randompoly import gen_random_poly sage: p = gen_random_poly(int(100)) sage: %timeit s = p.set() 100000 loops, best of 3: 2.85 µs per loop sage: %timeit d = p.deg() 100000 loops, best of 3: 2.26 µs per loop sage: %timeit m = p.lead() 100000 loops, best of 3: 6.7 µs per loop sage: %timeit n = p.navigation() 100000 loops, best of 3: 2.82 µs per loop sage: %timeit c = p.constant() 100000 loops, best of 3: 2.02 µs per loop
With the patch:
sage: %timeit s = p.set() 1000000 loops, best of 3: 540 ns per loop sage: %timeit d = p.deg() 1000000 loops, best of 3: 382 ns per loop sage: %timeit m = p.lead() 100000 loops, best of 3: 3.76 µs per loop sage: %timeit n = p.navigation() 1000000 loops, best of 3: 453 ns per loop sage: %timeit c = p.constant() 1000000 loops, best of 3: 305 ns per loop
comment:6 follow-ups: ↓ 7 ↓ 8 Changed 6 years ago by burcin
attachment:polybori-coercion.patch provides coercion enhancements and small fixes to pbori.pyx. It should be applied after attachment:polybori_booleanpolynomial_getattr.patch.
This patch adds coercion from ZZ, and GF(2) to BooleanPolynomialRing, so Martin's patch needs to be revised.
comment:7 in reply to: ↑ 6 Changed 6 years ago by burcin
- Cc burcin added
attachment:polybori-coercion.patch is now ticket #1667.
comment:8 in reply to: ↑ 6 Changed 6 years ago by malb
Replying to burcin:
This patch adds coercion from ZZ, and GF(2) to BooleanPolynomialRing, so Martin's patch needs to be revised.
In the sense that the comments are outdated?
comment:9 Changed 6 years ago by malb
The currRing.patch is now #1701 because it doesn't logically belong here.
comment:10 Changed 6 years ago by malb
- Summary changed from [with patch] improve PolyBoRi integration to [with patch, postive review] improve PolyBoRi integration
Deleted original patch, just use burcin's getattr patch.
comment:11 Changed 6 years ago by mabshoff
- Resolution set to fixed
- Status changed from new to closed
Merged in Sage 2.10.alpha0.
Caution: when patching the offset was very large: Hunk #1 succeeded at 1372 (offset 284 lines). I did verify that indeed the section was moved that far down in the current sources.
Cheers,
Michael
On sage.math, applied to alpha2: