Opened 4 years ago
Closed 4 years ago
#19521 closed defect (fixed)
wrong inverse action when using ConstructionFunctor.coercion_reversed
Reported by:  dkrenn  Owned by:  

Priority:  major  Milestone:  sage6.10 
Component:  coercion  Keywords:  
Cc:  Merged in:  
Authors:  Daniel Krenn  Reviewers:  Benjamin Hackl 
Report Upstream:  N/A  Work issues:  
Branch:  867d6d3 (Commits)  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 4 years ago by
comment:2 Changed 4 years ago by
 Dependencies set to #19259
comment:3 Changed 4 years ago by
 Branch set to u/dkrenn/coerce/inverseaction
comment:4 followup: ↓ 5 Changed 4 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/symbolicsubring' of trac.sagemath.org:sage into coerce/inverseaction

5638459  #19521: use coercion_reversed in InverseAction

comment:5 in reply to: ↑ 4 Changed 4 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 followup: ↓ 7 Changed 4 years ago by
 Branch changed from u/dkrenn/coerce/inverseaction to u/behackl/coercion/inverseaction
 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 thisor, 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/symbolicsubring

c4a0e22  merge accepting and rejecting functors in all cases

d376b10  revert changes to merge of functors

867d6d3  Merge branch 'u/behackl/symbolics/symbolicsubring' of git://trac.sagemath.org/sage into coercion/inverseaction

comment:7 in reply to: ↑ 6 Changed 4 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 thisor, at least, run
make ptestlong
myself.
Ok.
comment:8 Changed 4 years ago by
 Status changed from needs_review to positive_review
... alright. Passes ptestlong
and does what it should > positive_review
.
comment:9 Changed 4 years ago by
 Branch changed from u/behackl/coercion/inverseaction 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.