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: sage-6.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 dkrenn

BTW: For some reason it works (partially) for polynomial rings:

     sage: R.<x> = SR.subring(no_variables=True)[]
     sage: (x / 1).parent()
     Univariate Polynomial Ring in x over Symbolic Constants Subring

but

     sage: cm = sage.structure.element.get_coercion_model()
     sage: cm.explain(x, 1, operator.div)
     Action discovered.
        Right inverse action by Symbolic Ring on Univariate Polynomial Ring in x over Symbolic Constants Subring
        with precomposition on right by Conversion map:
          From: Integer Ring
          To:   Symbolic Ring
    Result lives in Univariate Polynomial Ring in x over Symbolic Ring
    Univariate Polynomial Ring in x over Symbolic Ring

At one point this should be investigated, but probably on a different ticket.

Last edited 4 years ago by dkrenn (previous) (diff)

comment:2 Changed 4 years ago by dkrenn

  • Authors set to Daniel Krenn
  • Dependencies set to #19259

comment:3 Changed 4 years ago by dkrenn

  • Branch set to u/dkrenn/coerce/inverse-action

comment:4 follow-up: Changed 4 years ago by dkrenn

  • Commit set to 563845928f4cfacb54f46156e59c12ffa46dbdab

Branch should fix this; still need to run a make ptestlong; will do this soon...


Last 10 new commits:

fb2885csubring in index.rst
eab9513simplify a doctest
23352b2change ValueError to TypeError to make everything work with SR as it should
2d8b7c4typo in docstring
15ec40edocstring of SR.subring
7938d95module description of subring
852959arename only_constants --> no_variables
e4837e9correct parent of result of an_element
6e5a098Merge 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 4 years ago by dkrenn

  • 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: Changed 4 years ago by behackl

  • 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:

ff99c7fTrac #19259: change to has_valid_variable
581f315Trac #19259: check validity of variables
c9b2428improve language
05dc834misc. changes, indentation, line breaks
8cc884afix merge of functors
aeae8f3Merge tag '7.0' into symbolics/symbolic-subring
c4a0e22merge accepting and rejecting functors in all cases
d376b10revert changes to merge of functors
867d6d3Merge branch 'u/behackl/symbolics/symbolic-subring' of git://trac.sagemath.org/sage into coercion/inverse-action

comment:7 in reply to: ↑ 6 Changed 4 years ago by dkrenn

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 4 years ago by behackl

  • 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 vbraun

  • Branch changed from u/behackl/coercion/inverse-action to 867d6d3cc606f4c291bc9098e490366160b40f42
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.