-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
Factory and pickling framework (part of coercion branch) #3623
Comments
comment:1
Code documented and works great/passes tests. Just need some doctests in factory.pyx (perhaps via a fake test class?) |
comment:3
Needs to be rebased against 3.1.2.alpha4:
Otherwise, I like this very much after having gone through the pain of implementing a unique factory for a parent already, I would have wasted a week less if this had already been in sage. |
comment:4
Thanks. I'll rebase this as soon as 3.1.2 comes out (as I doubt this ticket will make it into there). |
Attachment: 3623-factory-1.patch.gz |
Attachment: 3623-factory-2.patch.gz Attachment: 3623-factory-3.patch.gz |
comment:5
Attachment: 3623-factory-4.patch.gz All four patches rebased. |
Attachment: 3623-factory-5.patch.gz |
comment:7
Hi Robert, This bitrotted again. Sorry!
Can you rebase it an email me asap so this can get properly refereed and not bitrot again. |
patches 1-5 folded and rebased onto 3.2.1 |
comment:8
Attachment: 3623-all-3.2.1.patch.gz OK, this is once again rebased. |
comment:9
The patch applies cleanly against 3.2.1 and together with #4276 I am seeing two doctest failures:
I guess the first one is worrying while the rest is mostly a printing issue. The other failure is:
Cheers, Michael |
Attachment: 3623-fix.patch.gz apply after 3623-3.2.1-all.patch |
comment:10
OK, I fixed the doctests in coerce.pyx. The issue was that loads(dumps(V)) wasn't returning a new object anymore (this is good) so it wasn't testing what I wanted to test (equal but not identical parents). The other one is almost certainly due to #4276, looking into that now. |
comment:11
3623-fix.patch does indeed fix the problem and seem to not have any side effects, i.e. the doctest failure in ell_number_field.py is still there. No additional doctests did fail. Cheers, Michael |
comment:12
Ok, since 3623-all-3.2.1.patch and 3623-fix.patch + the two patches from #4276 make all tests pass I am giving this a positive review. There might still be issues here, so if anyone does take a closer look please open another ticket. The cost of this bitrotting is too high, so if this blows up there is only me and not Robert to blame. Cheers, Michael |
comment:13
Merged in Sage 3.2.2.alpha0 |
comment:14
One last remark: A couple doctests for the various "create_key" methods would be nice, but that can be done via a new ticket. Cheers, Michael |
comment:15
Thanks you. This one is a real pain to rebase, and I've wanted to use it elsewhere too. Note that Mike Hansen did look at these during Sage Days 10, and the response was generally positive (though not formal). |
Uniqueness of parents makes Sage operate much more smoothly. This leads to an enormous amount of nearly identical caching code scattered throughout the library. This factory handles all the caching for you, and also provides a good pickling mechanism.
Component: coercion
Issue created by migration from https://trac.sagemath.org/ticket/3623
The text was updated successfully, but these errors were encountered: