-
-
Notifications
You must be signed in to change notification settings - Fork 561
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
allow post-creation (pre-use) declaration of coercions #5598
Comments
comment:1
Attachment: 5598-coerce-declare.patch.gz Depends on #5597. |
comment:2
One can now do
This works as long as coercion has not yet been used (otherwise information cached (both here and elsewhere) becomes invalid). |
This comment has been minimized.
This comment has been minimized.
comment:5
Actually, though I wrote it on top of #5597, there's probably not a dependance. Note that I don't have any doctests yet (as it's completely unused functionality at this point). |
comment:6
Hence I'm physically incapable of giving this a positive review. Sorry. |
comment:7
Attachment: test-coercion.py.gz I have been using this patch (or actually a plain python version of it; see the sage-combinat patch queue) relatively intensively, and it does its job well. However it's hard to extract any interesting unitary test from those uses, since they all rely on the rest of the category stuff. I have just attached a stupid test instead ... It just plays with register_coercion, but should be easy to adapt to the others. Btw: robert: would it be possible to have register_coercion just raise a warning if coercions have readily been used? There are cases where it can be handy to abuse the system a bit from the interpreter. |
comment:8
Thanks for the tests. In retrospect, sometimes very simple tests like this are best at showing exactly what's going on. As for warnings vs. errors, yes it's possible, but I am strongly against it. If you need to abuse it, you can do so from Cython (or even write a spyx file that allows you to do so from the interpreter). There was talk about asserting coercions within contexts, but no one's gotten around to implementing them yet. |
comment:9
Hi Robert,
Yep. The construction of the parents and morphisms in this test are still ugly, though. Both ought to be essentially one-liners. Hopefuly things will be nicer when the category patch will be in.
My typical (ab)use case is someone constructing a couple parents in the interpreter, playing with them, and suddenly thinking "oh, but wouldn't all be natural if I added a coercion there?". The user will give it a try if this is trivial to do. Otherwise he won't and that's too bad. Btw: your patch changes the datastructure of parents to add the cython attribute
|
comment:10
This is all kinds of awesome, and yet totally useless. For example, the following fails:
Why? Because the What is to be done? I have no great ideas. Personally I would be happy with a flag to |
comment:11
Apply both robertwb, this badly needs documentation; it really can't be accepted without something. |
comment:12
Yes, I just haven't had time to work on any coercion-related stuff for a while. I'll be attacking issues like this during Sage days. |
Attachment: 5598-ncalexan-fixes-and-tests.patch.gz |
comment:13
Updated |
comment:14
5598-ncalexan-fixes-and-tests.patch seems malformed, could you try recreating this patch? |
Attachment: 5598-ncalexan-fixes-and-tests.2.patch.gz |
comment:15
Attachment: 5598-ncalexan-fixes-and-tests.3.patch.gz Ah, the mystery of the malformed patches. hg export -o DOESN'T overwrite an existing file, it appends! I think this is totally wrong, but I can work around it. This has been biting me for weeks, I now realize. Anyway, only tests.3.patch should be applied. |
comment:16
Attachment: 5598-test-coercion.patch.gz I made a patch out of test-coercion. Apply, in order
I give the tests/documentation a positive review, if someone is willing to give a +1 on the original patch we can get this ticket in. |
comment:17
It's not perfect but it's better than what was there before -- and doctested more than it was before! |
comment:18
Just a note: the added test adds a dependency on #5967. The later has a positive review, but Michael asked for a confirmation from a non sage-combinat person. Robert told me he would do this shortly. |
comment:19
I merged
and got the following failures:
|
Reviewer: Nick Alexander |
comment:20
Attachment: trac_5598.patch.gz I've attached trac_5598.patch which folds the above three together and is rebased for 4.2. I believe this should pass all tests. |
Author: Robert Bradshaw |
comment:22
The faliures above were just because in the tests "unset_coercions_used" should have been "_unset_coercions_used". All tests pass with trac_5598.patch. |
Merged: sage-4.2.alpha1 |
One can now do
This works as long as coercion has not yet been used (otherwise information cached (both here and elsewhere) becomes invalid).
CC: @nthiery
Component: coercion
Author: Robert Bradshaw
Reviewer: Nick Alexander
Merged: sage-4.2.alpha1
Issue created by migration from https://trac.sagemath.org/ticket/5598
The text was updated successfully, but these errors were encountered: