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)
Change History (9)
comment:1 Changed 7 years ago by
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
comment:3 follow-up: ↓ 4 Changed 7 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 in reply to: ↑ 3 Changed 7 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 7 years ago by
comment:5 follow-up: ↓ 6 Changed 7 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 in reply to: ↑ 5 Changed 7 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 7 years ago by
- Reviewers set to Alexander Dreyer
- Status changed from needs_review to positive_review
Patchbot agrees :)
comment:8 Changed 7 years ago by
- Merged in set to sage-5.7.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
The patch looks good, but what about the ordering?