Opened 10 years ago

Closed 10 years ago

#13965 closed defect (fixed)

BooleanPolynomialRing.remove_var(...) does not return a BooleanPolynomialRing

Reported by: Charles Bouillaguet Owned by: Martin Albrecht
Priority: major Milestone: sage-5.7
Component: commutative algebra Keywords:
Cc: Martin Albrecht, Alexander Dreyer, PolyBoRi team Merged in: sage-5.7.beta1
Authors: Charles Bouillaguet Reviewers: Alexander Dreyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

sage: P.<x,y,z,w> = BooleanPolynomialRing()
sage: P
Boolean PolynomialRing in x, y, z, w

and :

sage: P.remove_var('x')
Multivariate Polynomial Ring in y, z, w over Finite Field of size 2

Attachments (1)

13965_boolean_remove_var.patch (5.2 KB) - added by Charles Bouillaguet 10 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 10 years ago by Charles Bouillaguet

Authors: Charles Bouillaguet
Status: newneeds_review

comment:2 Changed 10 years ago by Alexander Dreyer

The patch looks good, but what about the ordering?

comment:3 Changed 10 years ago by Charles Bouillaguet

I mimicked what existed in multi_polynomial_ring_generic.pyx, and it ignored the order (my intuition would be that sometimes it must not be possible to carry on the order, when it is a block order for instance).

I think that we could let the user optionally provide an order for the "reduced" ring, and not try to do anything automatically... Do you agree ?

comment:4 in reply to:  3 Changed 10 years ago by Alexander Dreyer

Replying to Bouillaguet:

I mimicked what existed in multi_polynomial_ring_generic.pyx, and it ignored the order (my intuition would be that sometimes it must not be possible to carry on the order, when it is a block order for instance).

For block orders you just have to make the blocks smaller. That' not a oneliner, but something like

blks=R.term_order().blocks()
max_idx = len(blks[0])
if idx < max_idx:
  if nlen > 0:
    new_order = TermOrder(bl, len(bl)-1)
else:
  new_order = bl

for bl in blks[1:]:
  nlen = len(bl)
  max_idx += nlen
  if idx < max_idx:
     if nlen > 0:
       new_order += TermOrder(bl, nlen-1)
  else:
     new_order += bl
 

should do.

I think that we could let the user optionally provide an order for the "reduced" ring, and not try to do anything automatically... Do you agree ?

It the line above do not work out for some case, it would be an acceptable alternative.

Last edited 10 years ago by Alexander Dreyer (previous) (diff)

Changed 10 years ago by Charles Bouillaguet

comment:5 Changed 10 years ago by Charles Bouillaguet

I implemented a compromise solution, and for good measure I also adapted it to generic polynomial rings.

AlexanderDreyer?, do you like it?

comment:6 in reply to:  5 Changed 10 years ago by Alexander Dreyer

Replying to Bouillaguet:

I implemented a compromise solution, and for good measure I also adapted it to generic polynomial rings.

AlexanderDreyer?, do you like it?

Yeah, that indeed a good comprise: it works out of the box in typical use cases and is not to hard in the remaining cases (where people probably know their way thru). So this is a positive review, if patchbot agrees.

comment:7 Changed 10 years ago by Charles Bouillaguet

Reviewers: Alexander Dreyer
Status: needs_reviewpositive_review

Patchbot agrees :)

comment:8 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.7.beta1
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.