Opened 5 years ago
Closed 5 years ago
#19521 closed defect (fixed)
wrong inverse action when using ConstructionFunctor.coercion_reversed
Reported by: | dkrenn | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.10 |
Component: | coercion | Keywords: | |
Cc: | Merged in: | ||
Authors: | Daniel Krenn | Reviewers: | Benjamin Hackl |
Report Upstream: | N/A | Work issues: | |
Branch: | 867d6d3 (Commits, GitHub, GitLab) | Commit: | 867d6d3cc606f4c291bc9098e490366160b40f42 |
Dependencies: | #19259 | Stopgaps: |
Description
sage: Q.<y> = SR.subring(no_variables=True)[[]] sage: (y / 1).parent() Univariate Polynomial Ring in x over Symbolic Ring
but the result should be
Univariate Polynomial Ring in x over Symbolic Constants Subring
since there is a coercion from the integer ring to the symbolc constants subring.
This is because the attrirute coercion_reversed
of ConstructionFunctor
is not taken into account.
Change History (9)
comment:1 Changed 5 years ago by
comment:2 Changed 5 years ago by
- Dependencies set to #19259
comment:3 Changed 5 years ago by
- Branch set to u/dkrenn/coerce/inverse-action
comment:4 follow-up: ↓ 5 Changed 5 years ago by
- Commit set to 563845928f4cfacb54f46156e59c12ffa46dbdab
Branch should fix this; still need to run a make ptestlong
; will do this soon...
Last 10 new commits:
fb2885c | subring in index.rst
|
eab9513 | simplify a doctest
|
23352b2 | change ValueError to TypeError to make everything work with SR as it should
|
2d8b7c4 | typo in docstring
|
15ec40e | docstring of SR.subring
|
7938d95 | module description of subring
|
852959a | rename only_constants --> no_variables
|
e4837e9 | correct parent of result of an_element
|
6e5a098 | Merge branch 'u/dkrenn/symbolic-subring' of trac.sagemath.org:sage into coerce/inverse-action
|
5638459 | #19521: use coercion_reversed in InverseAction
|
comment:5 in reply to: ↑ 4 Changed 5 years ago by
- Status changed from new to needs_review
Replying to dkrenn:
Branch should fix this; still need to run a
make ptestlong
; will do this soon...
Passed. So I set it to needs review.
comment:6 follow-up: ↓ 7 Changed 5 years ago by
- Branch changed from u/dkrenn/coerce/inverse-action to u/behackl/coercion/inverse-action
- Commit changed from 563845928f4cfacb54f46156e59c12ffa46dbdab to 867d6d3cc606f4c291bc9098e490366160b40f42
- Reviewers set to Benjamin Hackl
Hi! I've merged the (positively reviewed) dependency into this branch and reviewed the code in this ticket.
I think I understand your fix as well as the strategy behind it, and I am willing to give this a positive_review
(i.e. everything LGTM). However, I'd rather be sure:
In principle, all that happens is that you order the functors and parents from smallest to largest (in the sense of coercion) and iterate over this reordered tower such that actually the smallest parent with the required property is found?
For example, (using your notation from the comment), if we had a tower A -> B <- C <- D -> E
, then the old code would iterate over [A, B, C, D, E, F]
, and in your fixed version we iterate over [A, D, C, B, E]
?
Also, I'd like to give the patchbots a chance to test this---or, at least, run make ptestlong
myself.
New commits:
ff99c7f | Trac #19259: change to has_valid_variable
|
581f315 | Trac #19259: check validity of variables
|
c9b2428 | improve language
|
05dc834 | misc. changes, indentation, line breaks
|
8cc884a | fix merge of functors
|
aeae8f3 | Merge tag '7.0' into symbolics/symbolic-subring
|
c4a0e22 | merge accepting and rejecting functors in all cases
|
d376b10 | revert changes to merge of functors
|
867d6d3 | Merge branch 'u/behackl/symbolics/symbolic-subring' of git://trac.sagemath.org/sage into coercion/inverse-action
|
comment:7 in reply to: ↑ 6 Changed 5 years ago by
Replying to behackl:
Hi! I've merged the (positively reviewed) dependency into this branch and reviewed the code in this ticket.
Thanks.
In principle, all that happens is that you order the functors and parents from smallest to largest (in the sense of coercion) and iterate over this reordered tower such that actually the smallest parent with the required property is found?
For example, (using your notation from the comment), if we had a tower
A -> B <- C <- D -> E
, then the old code would iterate over[A, B, C, D, E, F]
, and in your fixed version we iterate over[A, D, C, B, E]
?
Correct.
Also, I'd like to give the patchbots a chance to test this---or, at least, run
make ptestlong
myself.
Ok.
comment:8 Changed 5 years ago by
- Status changed from needs_review to positive_review
... alright. Passes ptestlong
and does what it should -> positive_review
.
comment:9 Changed 5 years ago by
- Branch changed from u/behackl/coercion/inverse-action to 867d6d3cc606f4c291bc9098e490366160b40f42
- Resolution set to fixed
- Status changed from positive_review to closed
BTW: For some reason it works (partially) for polynomial rings:
but
At one point this should be investigated, but probably on a different ticket.