-
-
Notifications
You must be signed in to change notification settings - Fork 556
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
Weaker precondition for registering a new coercion #7421
Comments
comment:2
I think is probably okay. After thinking about it for a bit, I could come up with a situation where this change would make the coercion graph non-commutatitve. I'll run all the tests with it here in a bit. |
comment:4
There is one way this can go wrong. Suppose one has B -> C, and one wants to register A -> B. If A -> C was previously requested, its non-existence will be cached. Now I don't think this will be an issue in practice, nor does |
This comment has been minimized.
This comment has been minimized.
comment:6
With the updated patch register_coercion also does not bark if _coercions_used is false. Otherwise, this triggered to failing doctests in sage/modules/fg_pid/fgp_module.py |
comment:7
Replying to @robertwb:
I agree. So, is this positive review? |
The previous version was missing a patch header. |
comment:8
Attachment: trac_7421-register_coercion_weaker_assertion.patch.gz I'm going to move this to 4.3 where it's more relevant. |
comment:9
Yes, positive review. |
Merged: sage-4.3.alpha0 |
With the attached patch, the precondition for registering a new
coercion from P to Q with register_coercion becomes:
"no coercion into P has been queried, or no coercion from P to Q has been registered or discovered earlier"
Which is a bit weaker than the previous:
"no coercion into P has been queried"
This should still be quite safe, while covering all the formerly
problematic practical use cases coming up in the category code #5981.
CC: @sagetrac-sage-combinat @robertwb
Component: coercion
Author: Nicolas M. Thiéry
Reviewer: Robert Bradshaw
Merged: sage-4.3.alpha0
Issue created by migration from https://trac.sagemath.org/ticket/7421
The text was updated successfully, but these errors were encountered: