#3738 closed enhancement (fixed)
[with patches, with two positive reviews] new coercion model
Reported by: | robertwb | Owned by: | robertwb |
---|---|---|---|
Priority: | major | Milestone: | sage-3.1 |
Component: | coercion | Keywords: | |
Cc: | Merged in: | ||
Authors: | Reviewers: | ||
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
This set of patches pulls the core coercion infrastructure from the coercion branch, without actually converting any of the Parents over. All Parents now descent from old_parent.Parent, which has a couple of compatibility routines.
With this in place Parents can be migrated one at a time. Other coercion branch features should be separate tickets.
Attachments (19)
Change History (43)
Changed 11 years ago by
Changed 11 years ago by
Changed 11 years ago by
Changed 11 years ago by
Changed 11 years ago by
Changed 11 years ago by
Changed 11 years ago by
Changed 11 years ago by
Changed 11 years ago by
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Summary changed from new coercion model to [with patches, needs review] new coercion model
comment:2 Changed 11 years ago by
- Summary changed from [with patches, needs review] new coercion model to [with patches, almost ready for review] new coercion model
Changed 11 years ago by
comment:3 Changed 11 years ago by
- Summary changed from [with patches, almost ready for review] new coercion model to [with patches, needs review] new coercion model
Changed 11 years ago by
Changed 11 years ago by
Changed 11 years ago by
comment:4 Changed 11 years ago by
Changed 11 years ago by
comment:5 Changed 11 years ago by
Added a bunch of doctests to parent.pyx
and coerce_maps.pyx
.
Changed 11 years ago by
comment:6 Changed 11 years ago by
Merged
- coerce.hg
- 3738-13-docs.patch
- 3738-fix-1.patch
in Sage 3.1.alpha2. These patches are on probation, but unless something goes wrong I see them in 3.1.final :)
By the way: "#3744 - Coercion between isomorphic parents should result in an element of the left operand's parent" is in the bundle and hence has also been merged.
Cheers,
Michael
comment:7 Changed 11 years ago by
With the patch applied I am seeing two doctest failures:
mabshoff@sage:/scratch/mabshoff/release-cycle/sage-3.1.alpha2$ sage -t -long devel/sage/sage/algebras/steenrod_algebra_element.py sage -t -long devel/sage/sage/algebras/steenrod_algebra_element.py ********************************************************************** File "/scratch/mabshoff/release-cycle/sage-3.1.alpha2/tmp/steenrod_algebra_element.py", line 218: sage: xm * y Expected: Sq^{5} Sq^{1} Got: Sq(3,1) ********************************************************************** 1 items had failures: 1 of 90 in __main__.example_0 ***Test Failed*** 1 failures. For whitespace errors, see the file /scratch/mabshoff/release-cycle/sage-3.1.alpha2/tmp/.doctest_steenrod_algebra_element.py [3.0 s] exit code: 1024
The above seems to be harmless.
The next one is:
sage -t -long devel/sage/sage/sets/set.py ********************************************************************** File "/scratch/mabshoff/release-cycle/sage-3.1.alpha2/tmp/set.py", line 442: sage: Set(ZZ).cardinality() Exception raised: Traceback (most recent call last): File "/scratch/mabshoff/release-cycle/sage-3.1.alpha2/local/lib/python2.5/doctest.py", line 1228, in __run compileflags, 1) in test.globs File "<doctest __main__.example_20[1]>", line 1, in <module> Set(ZZ).cardinality()###line 442: sage: Set(ZZ).cardinality() File "/scratch/mabshoff/release-cycle/sage-3.1.alpha2/local/lib/python2.5/site-packages/sage/sets/set.py", line 459, in cardinality raise NotImplementedError, "computation of cardinality of %s not yet implemented"%self.__object NotImplementedError: computation of cardinality of Integer Ring not yet implemented ********************************************************************** File "/scratch/mabshoff/release-cycle/sage-3.1.alpha2/tmp/set.py", line 816: sage: X.cardinality() Exception raised: Traceback (most recent call last): File "/scratch/mabshoff/release-cycle/sage-3.1.alpha2/local/lib/python2.5/doctest.py", line 1228, in __run compileflags, 1) in test.globs File "<doctest __main__.example_42[5]>", line 1, in <module> X.cardinality()###line 816: sage: X.cardinality() File "/scratch/mabshoff/release-cycle/sage-3.1.alpha2/local/lib/python2.5/site-packages/sage/sets/set.py", line 819, in cardinality return self.__X.cardinality() + self.__Y.cardinality() File "/scratch/mabshoff/release-cycle/sage-3.1.alpha2/local/lib/python2.5/site-packages/sage/sets/set.py", line 459, in cardinality raise NotImplementedError, "computation of cardinality of %s not yet implemented"%self.__object NotImplementedError: computation of cardinality of Integer Ring not yet implemented ********************************************************************** 2 items had failures: 1 of 5 in __main__.example_20 1 of 6 in __main__.example_42 ***Test Failed*** 2 failures. For whitespace errors, see the file /scratch/mabshoff/release-cycle/sage-3.1.alpha2/tmp/.doctest_set.py [3.4 s]
Cheers,
Michael
comment:8 Changed 11 years ago by
- Summary changed from [with patches, needs review] new coercion model to [with patches, needs review, under review by rlm and ncalexan] new coercion model
comment:9 Changed 11 years ago by
Thanks Nick and Robert for looking at this. Craig Citro has been reviewing it as well. I'm attaching a patch for the sets doctest failure.
Changed 11 years ago by
comment:10 Changed 11 years ago by
Spent a few hours of reading with rlm. We reviewed, quite carefully, patches 0, 1, 2, and 3.
I am confident that:
1) the new code is solid -- not necc. bug free, but fixable
2) the likelihood of this set of patches causing catastrophic failure is very low -- the sticky points (getattr :) appear to be covered.
I support applying this patch and moving forward with the conversion process 'in vivo'.
Changed 11 years ago by
comment:11 Changed 11 years ago by
- Summary changed from [with patches, needs review, under review by rlm and ncalexan] new coercion model to [with patches, needs review, with two positive reviews, under review by cc] new coercion model
I also support moving forward with these patches applied. It seems much more feasible to continue incrementally like this.
Changed 11 years ago by
comment:12 follow-up: ↓ 13 Changed 11 years ago by
Thank you for your good comments. I've incorporated some of the feedback in 3738-referee-comments.patch
There are no docs in generators.
True. See my earlier comments.
sage/structure/parent.pyx:128 - What is the syntax foo(bar=None, *, etc=None) doing? Does it throw away all but the first non-kwd arg?
http://www.python.org/dev/peps/pep-3102/
Why not always pass the parent (_unique morphism)?
There are two use cases, the first is speed for some critical types (Integer, Rational, RealDoubleElement?)--the PY_NEW needs a constructor that takes no arguments. The second is often the element constructor is a bound method, in which case the parent is implicitly already passed in.
sage/structure/parent.pyx:256 (typo) this it will always be
Fixed.
In the Parent class, why is it not the case that call delegates to coerce?
Coerce throws a (potentially expensive) error on failure.
sage/structure/parent.pyx:256 - needs to be wrapped in a list to prevent modification
Note sure what you mean here.
sage/structure/parent.pyx:~438 - Hom vs. hom should be explained in docs (definitions are there, but perhaps a warning)
Not sure I understand what you mean by warning. These functions (and their documentation) are mostly verbatim from the old Parent.
sage/structure/parent.pyx:597-9 etc. - should this be a copy?
You're right, though now these lists aren't modified near as often.
sage/structure/parent.pyx:530-1 - why not use the _generic_convert_map here? why does coerce_list use _generic_convert_map? what is the difference?
This should be a coercion, not a conversion. Also, Hom could do something special here. (This code is not mine, don't want to break it now.)
sage/structure/parent.pyx:807-24 - needs to be resolved
This needs to be thought through some more, but I think leaving the code and comments there does a good job of expressing our intent.
sage/structure/parent.pyx:839 - what does '"best"' mean? (868 - will this change in future patches?)
One that has the lowest _coerce_cost sum, but in reality it isn't used yet. I've changed the docstring to reflect this.
Can you clarify why _coerce_map_from_ is checked for overrides, but there is no such check for conversion?
Because _coerce_map_from_ and _has_coerce_map_from_ circularly call each other. I clarified this a bit more.
Why was coerce graph removed? :-(
It wasn't working. I want this too, but it should probably be a separate ticket.
sage/structure/category_object.pyx:500 WHY ARE YOU WRITING Pyrex?
That was old documentation, but I changed it.
All the comment blocks at the beginning of files are in the old style. i.e. copyright 2006... etc.
Lets fix this with another ticket.
Sometime it's pretty hard to figure out what's going on because there is some pretty damn random stuff with no docs or comments at all. (not a blocking comment, just saying...)
I tried to be really explicit with what I'm doing (especially the new stuff) but I won't claim to be perfect. Please ask if there is a particularly befuddling spot. A lot of parent is really, really old code too.
Why are inexact morphisms bad, Mommy?
Because they may not commute (e.g. due to rounding errors), and potentially loose information.
sage/structure/coerce_actions - Why are left and right different, my friends? Python?
As an example, let R be a non-commutative ring, and a \in R. Then a acts on elements in R[x] by multiplication on the left and by multiplication on the right.
sage/structure/coerce_actions.pyx:193 - typo "rikght"
Oops.
The inplace stuff needs more explanation.
Added comment. Currently the threshold is set to 0, so no inplace operations are used.
In coerce_map.pyx, "differs" should be "defers" in several places.
Oops again.
There are a lot of if-else statements which special case out things which are automatically handled by Python syntax. Is this for speed?
Probably, do you have an example?
sage/structure/coerce.pyx gets four thumbs up.
Thanks.
comment:13 in reply to: ↑ 12 Changed 11 years ago by
sage/structure/parent.pyx:256 - needs to be wrapped in a list to prevent modification
Note sure what you mean here.
There are a few cases where input lists are assigned as is, and then returned to the user (think of gens). That allows a user to say:
sage: x = R.gens() sage: x[0] = 0 sage: R.gens() *wrong*
sage/structure/coerce_actions - Why are left and right different, my friends? Python?
As an example, let R be a non-commutative ring, and a \in R. Then a acts on elements in R[x] by multiplication on the left and by multiplication on the right.
Sorry, that wasn't clear. One of the actions has in_place, the other does not. Why are they so different? Is it because there is a *= g but no g =* a?
There are a lot of if-else statements which special case out things which are automatically handled by Python syntax. Is this for speed?
Probably, do you have an example?
There is some code that checks if args is not None, and then kwds is not None, and dispatches f(x), f(x, *args), f(x, *args, kwds) accordingly. Seems odd.
comment:14 follow-up: ↓ 15 Changed 11 years ago by
sage/structure/parent.pyx:256 - needs to be wrapped in a list to prevent modification
Not sure what you mean here.
There are a few cases where input lists are assigned as is, and then returned to the user (think of gens)...
I think they're all OK now. Gens is not passed in this way for instance.
sage/structure/coerce_actions - Why are left and right different, my friends? Python?
As an example, let R be a non-commutative ring, and a \in R. Then a acts on elements in R[x] by multiplication on the left and by multiplication on the right.
Sorry, that wasn't clear. One of the actions has in_place, the other does not. Why are they so different? Is it because there is a *= g but no g =* a?
Yes, exactly.
There are a lot of if-else statements which special case out things which are automatically handled by Python syntax. Is this for speed?
Probably, do you have an example?
There is some code that checks if args is not None, and then kwds is not None, and dispatches f(x), f(x, *args), f(x, *args, kwds) accordingly. Seems odd.
Yes, this is for speed. Calling functions with variable-length arguments (and especially keywords) can be an non-negligible overhead for simple elements, and by by far the most common case is to have neither or just a single argument which the code is optimized for.
comment:15 in reply to: ↑ 14 Changed 11 years ago by
Replying to robertwb:
sage/structure/parent.pyx:256 - needs to be wrapped in a list to prevent modification
Not sure what you mean here.
There are a few cases where input lists are assigned as is, and then returned to the user (think of gens)...
I think they're all OK now. Gens is not passed in this way for instance.
I seem to recall a few instances where this happened which wasn't covered by your latest patch. After looking for a while I can't find any, but I am pretty tired.
I just want to say again, this code looks SOLID. Excellent work!
comment:16 Changed 11 years ago by
- Summary changed from [with patches, needs review, with two positive reviews, under review by cc] new coercion model to [with patches, needs review, with two positive reviews] new coercion model
I just want to pipe in and say that I'm really happy to see this getting merged. I haven't had as much time as I'd like to look through the code, but what I've seen looks good. I think that merging this and going from there is the way to go. My plan for refereeing is to start by writing some documentation about how exactly the new coercion code should be working, and seeing if it does as claimed. I'll start submitting some patches with more documentation as soon as I can, but since Nick and Robert have already looked over the first bits of this, I'm happy to say we merge it and go from there.
comment:17 Changed 11 years ago by
- Summary changed from [with patches, needs review, with two positive reviews] new coercion model to [with patches, with two positive reviews] new coercion model
comment:18 follow-up: ↓ 19 Changed 11 years ago by
Someone please review 3738-cardinality.patch and 3738-referee-comments.patch so that I can merge those two patches and close the ticket.
Cheers,
Michael
comment:19 in reply to: ↑ 18 Changed 11 years ago by
Replying to mabshoff:
Someone please review 3738-cardinality.patch and 3738-referee-comments.patch so that I can merge those two patches and close the ticket.
I'll give this a positive review. I haven't run tests with 3738-cardinality.patch
, but it's an obvious typo fix. The patch 3738-cardinality.patch
addresses all of the issues in the referee comments (which are both Nick's and mine, for the record).
+ 1
comment:20 Changed 11 years ago by
Rather,
The patch 3738-referee-comments.patch
addresses all of the issues in the referee comments (which are both Nick's and mine, for the record).
comment:21 Changed 11 years ago by
- Resolution set to fixed
- Status changed from new to closed
Merged 3738-cardinality.patch and 3738-referee-comments.patch in Sage 3.1.rc0.
Can someone please tell me how the credit should be assigned on this ticket? My impression:
Code: Robert Bradshaw, David Roe Review: Nick Alexander, Robert Miller, Craig Citro
Cheers,
Michael
comment:22 Changed 11 years ago by
I, ncalexan, am fine with review credit.
comment:23 Changed 11 years ago by
We need the following fix to make doctests pass with the patches applied:
--- a/sage/structure/parent.pyx Wed Aug 13 18:50:14 2008 -0700 +++ b/sage/structure/parent.pyx Thu Aug 14 15:03:16 2008 -0700 @@ -16,6 +16,8 @@ cimport element cimport sage.categories.morphism as morphism cimport sage.categories.map as map + +from copy import copy
Credit goes to William.
Cheers,
Michael
comment:24 Changed 11 years ago by
Re credit, that looks good to me.
A remark about doctest coverage: