Opened 7 years ago

Closed 7 years ago

#13965 closed defect (fixed)

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

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

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 Bouillaguet 7 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 7 years ago by Bouillaguet

  • Authors set to Charles Bouillaguet
  • Status changed from new to needs_review

comment:2 Changed 7 years ago by AlexanderDreyer

The patch looks good, but what about the ordering?

comment:3 follow-up: Changed 7 years ago by 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 7 years ago by AlexanderDreyer

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 7 years ago by AlexanderDreyer (previous) (diff)

Changed 7 years ago by Bouillaguet

comment:5 follow-up: Changed 7 years ago by 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 7 years ago by AlexanderDreyer

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 7 years ago by Bouillaguet

  • Reviewers set to Alexander Dreyer
  • Status changed from needs_review to positive_review

Patchbot agrees :)

comment:8 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.7.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.