Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove attempt to coerce scalar to basering for truediv #39371

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

nbruin
Copy link
Contributor

@nbruin nbruin commented Jan 23, 2025

As was observed in #39318, presently division of general univariate polynomials first tries to convert the dividing element into the base ring and invert it there. That's a problem with something like QQ['x']['y'] and computing y/1: the scalar 1 gets coerced into QQ['x'] and computing its inverse there yields an element in the field of fractions of QQ['x']. The parent of the result is therefore the polynomial ring in y over that.

The coercion framework is capable of finding an action of QQ on QQ['x']['y'], so if we just leave it to the coercion framework, we end up with a tighter parent for the result. Preliminary timings indicate that there is no significant performance regression, but more detailed timing might be warranted.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Jan 23, 2025

Documentation preview for this PR (built with commit a36436b; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@nbruin
Copy link
Contributor Author

nbruin commented Jan 24, 2025

Mentioning @tscrim for input. It looks like the change in parents caused by this PR cause some errors. Some of them might simply be a different message in something that was an error before and a few others may be exposing bugs that were previously hidden behind the old behaviour.

@nbruin
Copy link
Contributor Author

nbruin commented Jan 24, 2025

One of the doctest errors may come down to this:

sage: K=Qp(5)
sage: R.<x>=K[]
sage: coercion_model.explain(R,K,operator.truediv)
Coercion on right operand via
    Polynomial base injection morphism:
      From: 5-adic Field with capped relative precision 20
      To:   Univariate Polynomial Ring in x over 5-adic Field with capped relative precision 20
Arithmetic performed after coercions.
Result lives in Fraction Field of Univariate Polynomial Ring in x over 5-adic Field with capped relative precision 20
Fraction Field of Univariate Polynomial Ring in x over 5-adic Field with capped relative precision 20

Note that it does not discover an action of the base ring on the polynomial ring. Instead, it coerces to the polynomial ring, so now the result lies in the fraction field ... For contrast:

sage: coercion_model.explain(QQ['x'],QQ,operator.mul)
Action discovered.
    Right scalar multiplication by Rational Field on Univariate Polynomial Ring in x over Rational Field
Result lives in Univariate Polynomial Ring in x over Rational Field
Univariate Polynomial Ring in x over Rational Field

@nbruin
Copy link
Contributor Author

nbruin commented Jan 25, 2025

The problem seems to be here:

return None
# The code below has never been tested and is somehow subtly broken.

Apparently this functionality is just not implemented! it's a surprise that it works without the change on this ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant