Opened 7 years ago

Closed 7 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)

polybori_booleanpolynomial_getattr.patch (2.4 KB) - added by burcin 7 years ago.
Make the methods of BooleanPolynomial? in getattr directly available.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 7 years ago by rlm

  • Summary changed from [with patch] improve PolyBoRi integration to [with patch, negative review] improve PolyBoRi integration

On sage.math, applied to alpha2:

sage -t  devel/sage-main/sage/crypto/mq/sr.py               sh: line 1: 31648 Segmentation fault      /home/rlmill/release/sage-2.9.1.alpha3/local/bin/python .doctest_sr.py >.doctest/out 2>.doctest/err

A mysterious error (perphaps a memory error?) occurred, which may have crashed doctest.

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

Disregard those valgrind results- they are useless.

comment:4 Changed 7 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.

Changed 7 years ago by burcin

Make the methods of BooleanPolynomial? in getattr directly available.

comment:5 Changed 7 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: Changed 7 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 7 years ago by burcin

  • Cc burcin added

comment:8 in reply to: ↑ 6 Changed 7 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 7 years ago by malb

The currRing.patch is now #1701 because it doesn't logically belong here.

comment:10 Changed 7 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 7 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

Note: See TracTickets for help on using tickets.