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: |
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)
Change History (9)
comment:1 Changed 10 years ago by
Authors: | → Charles Bouillaguet |
---|---|
Status: | new → needs_review |
comment:2 Changed 10 years ago by
comment:3 follow-up: 4 Changed 10 years ago by
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 Changed 10 years ago by
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.
Changed 10 years ago by
Attachment: | 13965_boolean_remove_var.patch added |
---|
comment:5 follow-up: 6 Changed 10 years ago by
I implemented a compromise solution, and for good measure I also adapted it to generic polynomial rings.
AlexanderDreyer?, do you like it?
comment:6 Changed 10 years ago by
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
Reviewers: | → Alexander Dreyer |
---|---|
Status: | needs_review → positive_review |
Patchbot agrees :)
comment:8 Changed 10 years ago by
Merged in: | → sage-5.7.beta1 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
The patch looks good, but what about the ordering?