Sage: Ticket #3738: [with patches, with two positive reviews] new coercion model
https://trac.sagemath.org/ticket/3738
<p>
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.
</p>
<p>
With this in place Parents can be migrated one at a time. Other coercion branch features should be separate tickets.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/3738
Trac 1.1.6robertwbTue, 29 Jul 2008 09:18:47 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-0-generators.patch</em>
</li>
</ul>
TicketrobertwbTue, 29 Jul 2008 09:18:58 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-1-new_parent.patch</em>
</li>
</ul>
TicketrobertwbTue, 29 Jul 2008 09:19:11 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-2-new_parent_fixes.patch</em>
</li>
</ul>
TicketrobertwbTue, 29 Jul 2008 09:19:30 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-3-new_coercion_model.patch</em>
</li>
</ul>
TicketrobertwbTue, 29 Jul 2008 09:19:47 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-4-coerce_name_fixes.patch</em>
</li>
</ul>
TicketrobertwbTue, 29 Jul 2008 09:20:00 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-5-coerce_compat_layer.patch</em>
</li>
</ul>
TicketrobertwbTue, 29 Jul 2008 09:20:12 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-6-coerce_tests_pass.patch</em>
</li>
</ul>
TicketrobertwbTue, 29 Jul 2008 09:20:35 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-8-old_check.patch</em>
</li>
</ul>
TicketrobertwbTue, 29 Jul 2008 09:20:58 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-9-ccallmap.patch</em>
</li>
</ul>
TicketrobertwbTue, 29 Jul 2008 09:21:11 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-10-coerce_docs.patch</em>
</li>
</ul>
TicketrobertwbTue, 29 Jul 2008 09:22:05 GMTsummary changed
https://trac.sagemath.org/ticket/3738#comment:1
https://trac.sagemath.org/ticket/3738#comment:1
<ul>
<li><strong>summary</strong>
changed from <em>new coercion model</em> to <em>[with patches, needs review] new coercion model</em>
</li>
</ul>
TicketrobertwbTue, 29 Jul 2008 16:33:27 GMTsummary changed
https://trac.sagemath.org/ticket/3738#comment:2
https://trac.sagemath.org/ticket/3738#comment:2
<ul>
<li><strong>summary</strong>
changed from <em>[with patches, needs review] new coercion model</em> to <em>[with patches, almost ready for review] new coercion model</em>
</li>
</ul>
TicketrobertwbWed, 30 Jul 2008 04:21:35 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-7-maps.patch</em>
</li>
</ul>
TicketrobertwbWed, 30 Jul 2008 04:23:37 GMTsummary changed
https://trac.sagemath.org/ticket/3738#comment:3
https://trac.sagemath.org/ticket/3738#comment:3
<ul>
<li><strong>summary</strong>
changed from <em>[with patches, almost ready for review] new coercion model</em> to <em>[with patches, needs review] new coercion model</em>
</li>
</ul>
TicketrobertwbWed, 30 Jul 2008 05:53:43 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-12-explain-div.patch</em>
</li>
</ul>
TicketrobertwbWed, 30 Jul 2008 06:18:17 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-11-constructor.patch</em>
</li>
</ul>
TicketrobertwbWed, 30 Jul 2008 06:18:27 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>coerce.hg</em>
</li>
</ul>
TicketwasThu, 31 Jul 2008 16:48:36 GMT
https://trac.sagemath.org/ticket/3738#comment:4
https://trac.sagemath.org/ticket/3738#comment:4
<p>
A remark about doctest coverage:
</p>
<pre class="wiki">On Jul 30, 2008, at 9:49 AM, William Stein wrote:
>> I think this is probably something that it would be good for multiple
>> people could look at. (Both those previously involved in coercion,
>> and those not). It could help spread the work around too.
>>
>
> Michael Abshoff commented to me that the new coercion code you've
> posted introduces new functions that have no doctests. Any comment?
That is a true accusation, though I feel there is some justification
as many of the functions are not actually being used yet so it is
hard to test them (they were being used in the coercion branch, so
it's not completely untested code, they're just not used until
Parents are converted.) Also, a lot of them are generic helper
functions that are supposed to be overridden. And a third point is
that much of this was written before the 100% doctest rule, and there
is a significant portion that is renaming/re-factoring old code.
That being said, I have strived for better coverage (and
documentation, not just tests). Currently in terms of new (or heavily
modified) code we have:
sage/structure/coerce.pyx
SCORE sage/structure/coerce.pyx: 100% (20 of 20)
sage/structure/coerce_actions.pyx
ERROR: Please define a s == loads(dumps(s)) doctest.
SCORE sage/structure/coerce_actions.pyx: 66% (10 of 15)
Missing documentation:
* __init__(self, G, S)
* Element _call_(self, g, a)
* __init__(self, G, S)
* Element _call_(self, a, g)
* __cinit__(self)
The first 4 are methods of the RAction and LAction classes that are
not yet used anywhere in the Sage library (but code exists to
instantiate them if _l_action or _r_action is defined on the
elements). I'm not sure if/how __cinit__ should be tested.
sage/structure/coerce_dict.pyx
SCORE sage/structure/coerce_dict.pyx: 100% (14 of 14)
sage/structure/coerce_maps.pyx
ERROR: Please define a s == loads(dumps(s)) doctest.
SCORE sage/structure/coerce_maps.pyx: 28% (7 of 25)
Mostly __init__ and _call_ methods, but since no Parents have been
converted over they are never used (and some can't even be used until
we have converted Parents). There still some room for improvement
here, and I will write some more documentation for this file.
Boilerplate __init__ functions are particularly unenlightening to
doctest.
sage/structure/generators.pyx
SCORE sage/structure/generators.pyx: 11% (5 of 45)
[we don't use this yet, other then the fact that there's a cdef'd
slot to put the generators object. If coverage on this file is a
problem, I would go ahead and delete much of this file, and only put
back things as they are needed]
That is where things stand. I will go add more doctests to
coerce_maps.pyx (though I know I can't hit 100% until it's actually
used) but I think it would be good to start reviewing it, and I would
advocate that for this particular project it be grandfathered in to
some extent for practicalities sake. (This is not the case for future
coercion tickets, which should meet the 100% coverage standard.)
</pre>
TicketrobertwbSun, 03 Aug 2008 09:28:39 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-13-docs.patch</em>
</li>
</ul>
TicketrobertwbSun, 03 Aug 2008 09:30:15 GMT
https://trac.sagemath.org/ticket/3738#comment:5
https://trac.sagemath.org/ticket/3738#comment:5
<p>
Added a bunch of doctests to <code>parent.pyx</code> and <code>coerce_maps.pyx</code>.
</p>
TicketrobertwbWed, 06 Aug 2008 05:55:16 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-fix-1.patch</em>
</li>
</ul>
TicketmabshoffWed, 13 Aug 2008 22:15:25 GMT
https://trac.sagemath.org/ticket/3738#comment:6
https://trac.sagemath.org/ticket/3738#comment:6
<p>
Merged
</p>
<ul><li>coerce.hg
</li><li>3738-13-docs.patch
</li><li>3738-fix-1.patch
</li></ul><p>
in Sage 3.1.alpha2. These patches are on probation, but unless something goes wrong I see them in 3.1.final :)
</p>
<p>
By the way: "<a class="closed ticket" href="https://trac.sagemath.org/ticket/3744" title="defect: [with patch, positive review] Coercion between isomorphic parents ... (closed: fixed)">#3744</a> - 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.
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketmabshoffWed, 13 Aug 2008 22:37:14 GMT
https://trac.sagemath.org/ticket/3738#comment:7
https://trac.sagemath.org/ticket/3738#comment:7
<p>
With the patch applied I am seeing two doctest failures:
</p>
<pre class="wiki">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
</pre><p>
The above seems to be harmless.
</p>
<p>
The next one is:
</p>
<pre class="wiki">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]
</pre><p>
Cheers,
</p>
<p>
Michael
</p>
TicketrlmThu, 14 Aug 2008 02:06:16 GMTsummary changed
https://trac.sagemath.org/ticket/3738#comment:8
https://trac.sagemath.org/ticket/3738#comment:8
<ul>
<li><strong>summary</strong>
changed from <em>[with patches, needs review] new coercion model</em> to <em>[with patches, needs review, under review by rlm and ncalexan] new coercion model</em>
</li>
</ul>
TicketrobertwbThu, 14 Aug 2008 04:37:42 GMT
https://trac.sagemath.org/ticket/3738#comment:9
https://trac.sagemath.org/ticket/3738#comment:9
<p>
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.
</p>
TicketrobertwbThu, 14 Aug 2008 04:38:25 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-cardinality.patch</em>
</li>
</ul>
TicketncalexanThu, 14 Aug 2008 05:14:53 GMT
https://trac.sagemath.org/ticket/3738#comment:10
https://trac.sagemath.org/ticket/3738#comment:10
<p>
Spent a few hours of reading with rlm. We reviewed, quite carefully, patches 0, 1, 2, and 3.
</p>
<p>
I am confident that:
</p>
<p>
1) the new code is solid -- not necc. bug free, but fixable
</p>
<p>
2) the likelihood of this set of patches causing catastrophic failure is very low -- the sticky points (<span class="underline">getattr</span> :) appear to be covered.
</p>
<p>
I support applying this patch and moving forward with the conversion process 'in vivo'.
</p>
TicketrlmThu, 14 Aug 2008 05:15:32 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>coercion_comments1.txt</em>
</li>
</ul>
TicketrlmThu, 14 Aug 2008 05:17:01 GMTsummary changed
https://trac.sagemath.org/ticket/3738#comment:11
https://trac.sagemath.org/ticket/3738#comment:11
<ul>
<li><strong>summary</strong>
changed from <em>[with patches, needs review, under review by rlm and ncalexan] new coercion model</em> to <em>[with patches, needs review, with two positive reviews, under review by cc] new coercion model</em>
</li>
</ul>
<p>
I also support moving forward with these patches applied. It seems much more feasible to continue incrementally like this.
</p>
TicketrobertwbThu, 14 Aug 2008 06:13:16 GMTattachment set
https://trac.sagemath.org/ticket/3738
https://trac.sagemath.org/ticket/3738
<ul>
<li><strong>attachment</strong>
set to <em>3738-referee-comments.patch</em>
</li>
</ul>
TicketrobertwbThu, 14 Aug 2008 06:15:41 GMT
https://trac.sagemath.org/ticket/3738#comment:12
https://trac.sagemath.org/ticket/3738#comment:12
<p>
Thank you for your good comments. I've incorporated some of the feedback in 3738-referee-comments.patch
</p>
<blockquote class="citation">
<p>
There are no docs in generators.
</p>
</blockquote>
<p>
True. See my earlier comments.
</p>
<blockquote class="citation">
<p>
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?
</p>
</blockquote>
<p>
<a class="ext-link" href="http://www.python.org/dev/peps/pep-3102/"><span class="icon"></span>http://www.python.org/dev/peps/pep-3102/</a>
</p>
<blockquote class="citation">
<p>
Why not always pass the parent (_unique morphism)?
</p>
</blockquote>
<p>
There are two use cases, the first is speed for some critical types (Integer, Rational, <a class="missing wiki">RealDoubleElement?</a>)--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.
</p>
<blockquote class="citation">
<p>
sage/structure/parent.pyx:256 (typo) this it will always be
</p>
</blockquote>
<p>
Fixed.
</p>
<blockquote class="citation">
<p>
In the Parent class, why is it not the case that <span class="underline">call</span> delegates to coerce?
</p>
</blockquote>
<p>
Coerce throws a (potentially expensive) error on failure.
</p>
<blockquote class="citation">
<p>
sage/structure/parent.pyx:256 - needs to be wrapped in a list to prevent modification
</p>
</blockquote>
<p>
Note sure what you mean here.
</p>
<blockquote class="citation">
<p>
sage/structure/parent.pyx:~438 - Hom vs. hom should be explained in docs (definitions are there, but perhaps a warning)
</p>
</blockquote>
<p>
Not sure I understand what you mean by warning. These functions (and their documentation) are mostly verbatim from the old Parent.
</p>
<blockquote class="citation">
<p>
sage/structure/parent.pyx:597-9 etc. - should this be a copy?
</p>
</blockquote>
<p>
You're right, though now these lists aren't modified near as often.
</p>
<blockquote class="citation">
<p>
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?
</p>
</blockquote>
<p>
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.)
</p>
<blockquote class="citation">
<p>
sage/structure/parent.pyx:807-24 - needs to be resolved
</p>
</blockquote>
<p>
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.
</p>
<blockquote class="citation">
<p>
sage/structure/parent.pyx:839 - what does '"best"' mean? (868 - will this change in future patches?)
</p>
</blockquote>
<p>
One that has the lowest _coerce_cost sum, but in reality it isn't used yet. I've changed the docstring to reflect this.
</p>
<blockquote class="citation">
<p>
Can you clarify why _coerce_map_from_ is checked for overrides, but there is no such check for conversion?
</p>
</blockquote>
<p>
Because _coerce_map_from_ and _has_coerce_map_from_ circularly call each other. I clarified this a bit more.
</p>
<blockquote class="citation">
<p>
Why was coerce graph removed? :-(
</p>
</blockquote>
<p>
It wasn't working. I want this too, but it should probably be a separate ticket.
</p>
<blockquote class="citation">
<p>
sage/structure/category_object.pyx:500 WHY ARE YOU WRITING Pyrex?
</p>
</blockquote>
<p>
That was old documentation, but I changed it.
</p>
<blockquote class="citation">
<p>
All the comment blocks at the beginning of files are in the old style. i.e. copyright 2006... etc.
</p>
</blockquote>
<p>
Lets fix this with another ticket.
</p>
<blockquote class="citation">
<p>
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...)
</p>
</blockquote>
<p>
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.
</p>
<blockquote class="citation">
<p>
Why are inexact morphisms bad, Mommy?
</p>
</blockquote>
<p>
Because they may not commute (e.g. due to rounding errors), and potentially loose information.
</p>
<blockquote class="citation">
<p>
sage/structure/coerce_actions - Why are left and right different, my friends? Python?
</p>
</blockquote>
<p>
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.
</p>
<blockquote class="citation">
<p>
sage/structure/coerce_actions.pyx:193 - typo "rikght"
</p>
</blockquote>
<p>
Oops.
</p>
<blockquote class="citation">
<p>
The inplace stuff needs more explanation.
</p>
</blockquote>
<p>
Added comment. Currently the threshold is set to 0, so no inplace operations are used.
</p>
<blockquote class="citation">
<p>
In coerce_map.pyx, "differs" should be "defers" in several places.
</p>
</blockquote>
<p>
Oops again.
</p>
<blockquote class="citation">
<p>
There are a lot of if-else statements which special case out things which are automatically handled by Python syntax. Is this for speed?
</p>
</blockquote>
<p>
Probably, do you have an example?
</p>
<blockquote class="citation">
<p>
sage/structure/coerce.pyx gets four thumbs up.
</p>
</blockquote>
<p>
Thanks.
</p>
TicketncalexanThu, 14 Aug 2008 06:51:32 GMT
https://trac.sagemath.org/ticket/3738#comment:13
https://trac.sagemath.org/ticket/3738#comment:13
<blockquote class="citation">
<blockquote class="citation">
<p>
sage/structure/parent.pyx:256 - needs to be wrapped in a list to prevent modification
</p>
</blockquote>
<p>
Note sure what you mean here.
</p>
</blockquote>
<p>
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:
</p>
<p>
sage: x = R.gens()
sage: x[0] = 0
sage: R.gens()
*wrong*
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
sage/structure/coerce_actions - Why are left and right different, my friends? Python?
</p>
</blockquote>
<p>
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.
</p>
</blockquote>
<p>
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?
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
There are a lot of if-else statements which special case out things which are automatically handled by Python syntax. Is this for speed?
</p>
</blockquote>
<p>
Probably, do you have an example?
</p>
</blockquote>
<p>
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, <strong>kwds) accordingly. Seems odd.
</strong></p>
TicketrobertwbThu, 14 Aug 2008 07:17:23 GMT
https://trac.sagemath.org/ticket/3738#comment:14
https://trac.sagemath.org/ticket/3738#comment:14
<blockquote class="citation">
<blockquote class="citation">
<blockquote class="citation">
<p>
sage/structure/parent.pyx:256 - needs to be wrapped in a list to prevent modification
</p>
</blockquote>
</blockquote>
</blockquote>
<blockquote class="citation">
<blockquote class="citation">
<p>
Not sure what you mean here.
</p>
</blockquote>
</blockquote>
<blockquote class="citation">
<p>
There are a few cases where input lists are assigned as is, and then returned to the user (think of gens)...
</p>
</blockquote>
<p>
I think they're all OK now. Gens is not passed in this way for instance.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
sage/structure/coerce_actions - Why are left and right different, my friends? Python?
</p>
</blockquote>
</blockquote>
<blockquote class="citation">
<blockquote class="citation">
<blockquote>
<p>
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.
</p>
</blockquote>
</blockquote>
</blockquote>
<blockquote class="citation">
<p>
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?
</p>
</blockquote>
<p>
Yes, exactly.
</p>
<blockquote class="citation">
<blockquote class="citation">
<blockquote class="citation">
<p>
There are a lot of if-else statements which special case out things which are automatically handled by Python syntax. Is this for speed?
</p>
</blockquote>
</blockquote>
</blockquote>
<blockquote class="citation">
<blockquote class="citation">
<p>
Probably, do you have an example?
</p>
</blockquote>
</blockquote>
<blockquote class="citation">
<p>
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, <strong>kwds) accordingly. Seems odd.
</strong></p>
</blockquote>
<p>
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.
</p>
TicketrlmThu, 14 Aug 2008 07:53:13 GMT
https://trac.sagemath.org/ticket/3738#comment:15
https://trac.sagemath.org/ticket/3738#comment:15
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/3738#comment:14" title="Comment 14">robertwb</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<blockquote class="citation">
<blockquote class="citation">
<p>
sage/structure/parent.pyx:256 - needs to be wrapped in a list to prevent modification
</p>
</blockquote>
</blockquote>
</blockquote>
<blockquote class="citation">
<blockquote class="citation">
<p>
Not sure what you mean here.
</p>
</blockquote>
</blockquote>
<blockquote class="citation">
<p>
There are a few cases where input lists are assigned as is, and then returned to the user (think of gens)...
</p>
</blockquote>
<p>
I think they're all OK now. Gens is not passed in this way for instance.
</p>
</blockquote>
<p>
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.
</p>
<p>
I just want to say again, this code looks <span class="underline">SOLID</span>. Excellent work!
</p>
TicketcraigcitroThu, 14 Aug 2008 10:14:15 GMTsummary changed
https://trac.sagemath.org/ticket/3738#comment:16
https://trac.sagemath.org/ticket/3738#comment:16
<ul>
<li><strong>summary</strong>
changed from <em>[with patches, needs review, with two positive reviews, under review by cc] new coercion model</em> to <em>[with patches, needs review, with two positive reviews] new coercion model</em>
</li>
</ul>
<p>
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.
</p>
TicketcraigcitroThu, 14 Aug 2008 10:14:43 GMTsummary changed
https://trac.sagemath.org/ticket/3738#comment:17
https://trac.sagemath.org/ticket/3738#comment:17
<ul>
<li><strong>summary</strong>
changed from <em>[with patches, needs review, with two positive reviews] new coercion model</em> to <em>[with patches, with two positive reviews] new coercion model</em>
</li>
</ul>
TicketmabshoffThu, 14 Aug 2008 16:47:41 GMT
https://trac.sagemath.org/ticket/3738#comment:18
https://trac.sagemath.org/ticket/3738#comment:18
<p>
Someone please review 3738-cardinality.patch and 3738-referee-comments.patch so that I can merge those two patches and close the ticket.
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketrlmThu, 14 Aug 2008 16:51:06 GMT
https://trac.sagemath.org/ticket/3738#comment:19
https://trac.sagemath.org/ticket/3738#comment:19
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/3738#comment:18" title="Comment 18">mabshoff</a>:
</p>
<blockquote class="citation">
<p>
Someone please review 3738-cardinality.patch and 3738-referee-comments.patch so that I can merge those two patches and close the ticket.
</p>
</blockquote>
<p>
I'll give this a positive review. I haven't run tests with <code>3738-cardinality.patch</code>, but it's an obvious typo fix. The patch <code>3738-cardinality.patch</code> addresses all of the issues in the referee comments (which are both Nick's and mine, for the record).
</p>
<blockquote>
<p>
+ 1
</p>
</blockquote>
TicketrlmThu, 14 Aug 2008 16:55:33 GMT
https://trac.sagemath.org/ticket/3738#comment:20
https://trac.sagemath.org/ticket/3738#comment:20
<p>
Rather,
</p>
<p>
The patch <code>3738-referee-comments.patch</code> addresses all of the issues in the referee comments (which are both Nick's and mine, for the record).
</p>
TicketmabshoffThu, 14 Aug 2008 16:57:44 GMTstatus changed; resolution set
https://trac.sagemath.org/ticket/3738#comment:21
https://trac.sagemath.org/ticket/3738#comment:21
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
</ul>
<p>
Merged 3738-cardinality.patch and 3738-referee-comments.patch in Sage 3.1.rc0.
</p>
<p>
Can someone please tell me how the credit should be assigned on this ticket? My impression:
</p>
<p>
Code: Robert Bradshaw, David Roe
Review: Nick Alexander, Robert Miller, Craig Citro
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketncalexanThu, 14 Aug 2008 17:22:16 GMT
https://trac.sagemath.org/ticket/3738#comment:22
https://trac.sagemath.org/ticket/3738#comment:22
<p>
I, ncalexan, am fine with review credit.
</p>
TicketmabshoffThu, 14 Aug 2008 22:11:49 GMT
https://trac.sagemath.org/ticket/3738#comment:23
https://trac.sagemath.org/ticket/3738#comment:23
<p>
We need the following fix to make doctests pass with the patches applied:
</p>
<pre class="wiki">--- 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
</pre><p>
Credit goes to William.
</p>
<p>
Cheers,
</p>
<p>
Michael
</p>
TicketrobertwbFri, 15 Aug 2008 07:23:49 GMT
https://trac.sagemath.org/ticket/3738#comment:24
https://trac.sagemath.org/ticket/3738#comment:24
<p>
Re credit, that looks good to me.
</p>
Ticket