Sage: Ticket #9880: Segfault in PyNaC 0.2.0.p4
https://trac.sagemath.org/ticket/9880
<p>
Here is a short example found by Burcin and reproducing the bug:
</p>
<pre class="wiki">b = [var('b_%s'%i) for i in range(4)]
precomp = (2^b_2 + 2)*(2^b_1 + 2^(-b_1) + 2^b_1*2^b_0 - 2^b_1*2^(-b_0)
- 2^(-b_1)*2^b_0 - 2^(-b_1)*2^(-b_0) + 2^b_0 + 2^(-b_0) - 9) + (2^b_1 +
2^(-b_1) + 2^b_1*2^b_0 - 2^b_1*2^(-b_0) - 2^(-b_1)*2^b_0 -
2^(-b_1)*2^(-b_0) + 2^b_0 + 2^(-b_0) - 9)/2^b_2
repl_dict = {b_0: b_0, b_3: b_1, b_2: b_3, b_1: b_2}
P = precomp.substitute(repl_dict)
P.expand()
</pre><p>
This is already being discussed here:
<a class="ext-link" href="http://groups.google.com/group/sage-support/browse_thread/thread/7c85f02c76012722"><span class="icon"></span>http://groups.google.com/group/sage-support/browse_thread/thread/7c85f02c76012722</a>
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/9880
Trac 1.2Burcin ErocalThu, 09 Sep 2010 09:06:04 GMTdescription changed; milestone set
https://trac.sagemath.org/ticket/9880#comment:1
https://trac.sagemath.org/ticket/9880#comment:1
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=1">diff</a>)
</li>
<li><strong>milestone</strong>
set to <em>sage-4.6</em>
</li>
</ul>
TicketJean-Pierre FloriWed, 29 Sep 2010 14:10:17 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:2
https://trac.sagemath.org/ticket/9880#comment:2
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=2">diff</a>)
</li>
</ul>
<p>
The bug happened because of the comparison functions which are used in a call to std::sort.
</p>
<p>
I have finally looked at the comparison functions and exchanging :
</p>
<pre class="wiki">cmpval = seq[0].coeff.compare(other.exponent);
</pre><p>
by
</p>
<pre class="wiki">cmpval = -seq[0].coeff.compare(other.exponent);
</pre><p>
in mul::compare_pow (mul.cpp:1265) seems to prevent the above bug from happening.
</p>
<p>
It seems to fit better with the change made by William Stein in power::compare_same_type (power.cpp:951).
</p>
<p>
However it doesn't mean the problem is completely solved...
</p>
<p>
I'll try to take a deeper look at the comparison functions at some point.
</p>
<p>
I tested the above fix with pynac 0.2.1.
</p>
TicketJean-Pierre FloriMon, 11 Oct 2010 14:51:48 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac9880_pynac_order_burcin_original.patch</em>
</li>
</ul>
<p>
Burcin original patch
</p>
TicketJean-Pierre FloriMon, 11 Oct 2010 14:52:10 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac9880-pynac_order_jp_new.patch</em>
</li>
</ul>
<p>
Patch to apply on top of the other one
</p>
TicketJean-Pierre FloriMon, 11 Oct 2010 14:57:20 GMT
https://trac.sagemath.org/ticket/9880#comment:3
https://trac.sagemath.org/ticket/9880#comment:3
<p>
We've been working on a patch to fix the issue.
</p>
<p>
Original discussion is here:
</p>
<p>
<a class="ext-link" href="http://groups.google.com/group/pynac-devel/browse_thread/thread/a36020bf9208bf08/abdf6671ef0b926a#abdf6671ef0b926a"><span class="icon"></span>http://groups.google.com/group/pynac-devel/browse_thread/thread/a36020bf9208bf08/abdf6671ef0b926a#abdf6671ef0b926a</a>
</p>
<p>
Burcin produced a patch restoring GiNaC original order for internal use and using the new ones only for printing ; thus fixing the bug.
</p>
<p>
I then worked on top of it to get a more consistent order.
</p>
<p>
You can test them using pynac 0.2.1 from <a class="ext-link" href="http://trac.sagemath.org/sage_trac/search/opensearch?q=ticket%3A9901"><span class="icon"></span>#9901</a> ("cd spkg/standard/pynac-0.2.1/src/" then "hg import" both patches and build/install, you should "./sage -b" if upgrading from a previous version of pynac).
</p>
<p>
I don't consider that version as definitve, but would like to get some feedback on the order used for printing.
</p>
<p>
I don't use everything pynac provides so it is more than probable that some expression are not correctly printed now.
</p>
<p>
Do not hesitate to report it.
</p>
TicketJean-Pierre FloriMon, 15 Nov 2010 11:01:04 GMTstatus changed
https://trac.sagemath.org/ticket/9880#comment:4
https://trac.sagemath.org/ticket/9880#comment:4
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketKarl-Dieter CrismanThu, 18 Nov 2010 16:11:14 GMTcc set
https://trac.sagemath.org/ticket/9880#comment:5
https://trac.sagemath.org/ticket/9880#comment:5
<ul>
<li><strong>cc</strong>
<em>Karl-Dieter Crisman</em> <em>Jason Grout</em> added
</li>
</ul>
<p>
<a class="closed ticket" href="https://trac.sagemath.org/ticket/10282" title="#10282: defect: Discrepancy in symbol ordering in multiplication (closed: duplicate)">#10282</a> almost certainly is the same thing.
</p>
TicketKarl-Dieter CrismanThu, 18 Nov 2010 16:18:04 GMTcc changed
https://trac.sagemath.org/ticket/9880#comment:6
https://trac.sagemath.org/ticket/9880#comment:6
<ul>
<li><strong>cc</strong>
<em>Jason Grout</em> removed
</li>
</ul>
<p>
Fixing this probably will close <a class="closed ticket" href="https://trac.sagemath.org/ticket/9632" title="#9632: defect: System-dependent term order for printed expressions (closed: fixed)">#9632</a> - just putting that here for the record.
</p>
TicketVolker BraunWed, 24 Nov 2010 01:20:48 GMT
https://trac.sagemath.org/ticket/9880#comment:7
https://trac.sagemath.org/ticket/9880#comment:7
<p>
Apply <code>trac_9880_revert_marking_random_from_trac_10187.patch</code> to re-enable doctests that fail on OSX/PPC.
</p>
TicketKarl-Dieter CrismanWed, 24 Nov 2010 03:39:11 GMT
https://trac.sagemath.org/ticket/9880#comment:8
https://trac.sagemath.org/ticket/9880#comment:8
<p>
(Just OS X, not any particular architecture, I think.)
</p>
TicketVolker BraunWed, 01 Dec 2010 14:07:35 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880_revert_marking_random_from_trac_10187.patch</em>
</li>
</ul>
<p>
Re-enable doctests that fail on PPC due to this issue (updated)
</p>
TicketKarl-Dieter CrismanThu, 13 Jan 2011 16:08:17 GMT
https://trac.sagemath.org/ticket/9880#comment:9
https://trac.sagemath.org/ticket/9880#comment:9
<blockquote class="citation">
<p>
I don't use everything pynac provides so it is more than probable that some expression are not correctly printed now.
</p>
<p>
Do not hesitate to report it.
</p>
</blockquote>
<p>
I'd like to test this at some point, but have two problems.
</p>
<ul><li>There is too much C++ for me to review it properly, unless a lot of it really is just reverting. Is there an easy way to figure out what is actual new code, and what is going back to something more-or-less Ginac?
</li></ul><ul><li>I am not sure exactly what sort of expressions would not be properly printed. Can you give any kind of example of what sort of bad behavior to look for with testing (perhaps randomized)?
</li></ul>
TicketJean-Pierre FloriThu, 13 Jan 2011 16:34:56 GMT
https://trac.sagemath.org/ticket/9880#comment:10
https://trac.sagemath.org/ticket/9880#comment:10
<p>
I guess there are two main parts in this ticket, I did not have a look for a long time, so all this should be taken carefully :
</p>
<ul><li>Burcin patch which (more or less) revert the internal ordering in Pynac to the original one in Ginac and create new methods to use a different order for printing. We could maybe only apply the part reverting the internal order to Ginac original one to get the bug solved and close this ticket.
</li></ul><ul><li>However, it is not a solution to use Ginac order for printing because it is quite random and unpredictable, it depends on variable order creation among others. That's the reason for the different order for printing (and getting operands and so on). It was not quite coherent, whence my patch to make it a little better. We could merge that in a second ticket. By the way, the order implemented right now should be more or less degrevlex.
</li></ul><ul><li>With that new framework, we could also quite easily allow the use of different orders for manipulating (ie printing, getting operands...) symbolic expressions in Sage (Burcin already mentionned that somewhere on the Web IIRC).
</li></ul><ul><li>I think there is still work to do regarding expressions manipulation (see <a class="closed ticket" href="https://trac.sagemath.org/ticket/9989" title="#9989: enhancement: easier access to operands of a symbolic expression (closed: fixed)">#9989</a>)
</li></ul><ul><li>I don't really have an idea of what could get misprinted with Burcin+my patch applied. You could try multivariate polynomials to check it follows degrevlex, then such expressions times exponentials and let me know what you feel.<br />
</li></ul>
TicketVolker BraunThu, 13 Jan 2011 16:36:11 GMT
https://trac.sagemath.org/ticket/9880#comment:11
https://trac.sagemath.org/ticket/9880#comment:11
<p>
I could review the c++ but the code in the ticket seems to work for me. If it segfaults for you, how about including a full sage session with a stack backtrace at the very least?
</p>
TicketJean-Pierre FloriThu, 13 Jan 2011 16:47:42 GMT
https://trac.sagemath.org/ticket/9880#comment:12
https://trac.sagemath.org/ticket/9880#comment:12
<p>
There is all you are asking for in the original discussion linked in the ticket description.
</p>
<p>
Maybe something changed since then, the ticket is quite old.
</p>
TicketJean-Pierre FloriThu, 13 Jan 2011 17:15:34 GMT
https://trac.sagemath.org/ticket/9880#comment:13
https://trac.sagemath.org/ticket/9880#comment:13
<p>
As far as I'm concerned, it still segfaults with the Sage 4.6 package for 32-bit Ubuntu provided on sagemath.org, as well as on a 64 bit Sage 4.6 that I built myself.
</p>
<p>
I did not test it any 4.6.1 alpha or rc yet.
</p>
TicketJean-Pierre FloriFri, 14 Jan 2011 07:31:44 GMT
https://trac.sagemath.org/ticket/9880#comment:14
https://trac.sagemath.org/ticket/9880#comment:14
<p>
I finally got Sage 4.6.1 final built from scratch, and:
</p>
<ul><li>indeed the specific instance of the bug mentionned here is no longer present
</li><li>however, it does not mean that the bug is solved because:
<ul><li>nothing changed in pynac (IIRC)
</li><li>the problem in the ordering used by pynac (which caused the bug when called within std::sort) are still present, I'll post a problematic expression asap
</li></ul></li></ul><p>
By the way, the problem of different ordering on different architecture should still be present (10187#comment:39). <br />
</p>
TicketJean-Pierre FloriFri, 14 Jan 2011 09:36:42 GMT
https://trac.sagemath.org/ticket/9880#comment:15
https://trac.sagemath.org/ticket/9880#comment:15
<p>
Ok, here is a kind of strange example for the problems of ordering still hapenning with Sage 4.6.1:
</p>
<pre class="wiki">sage: b_0,b_1,b_2=var('b_0,b_1,b_2')
sage: f = 1/27*b_2^2/(2^b_2)^2 + 1/27*b_1^2/(2^b_1)^2 + 1/27*b_0^2/(2^b_0)^2 + 1/27*b_2/(2^b_2)^2 - 2/81/(2^b_2)^2 + 1/27*b_1/(2^b_1)^2 + 8/243/(2^b_2)^2 - 1/81*b_0/(2^b_0)^2 - 1/27*b_1^2/((2^b_2)^2*(2^b_1)^2) - 1/27*b_0^2/((2^b_2)^2*(2^b_0)^2) - 20/243/(2^b_1)^2 + 1/9/2^b_0 + 4/81*b_0/(2^b_0)^2 - 8/243/(2^b_2)^2 - 2/9/(2^b_2*2^b_1) - 2/9/(2^b_2*2^b_0) + 8/243/(2^b_1)^2 - 1/9/2^b_0 + 2/9/(2^b_2*2^b_1) + 2/9/(2^b_2*2^b_0) - 2/27*b_1*b_2/((2^b_2)^2*(2^b_1)^2) - 1/27*b_2^2/((2^b_2)^2*(2^b_1)^2) - 2/27*b_0*b_2/((2^b_2)^2*(2^b_0)^2) - 1/27*b_2^2/((2^b_2)^2*(2^b_0)^2) + 2/81/(2^b_1)^2 - 1/27*b_0^2/((2^b_1)^2*(2^b_0)^2) - 2/27*b_0*b_1/((2^b_1)^2*(2^b_0)^2) - 1/27*b_1^2/((2^b_1)^2*(2^b_0)^2) - 2/81/(2^b_0)^2 + 5/27*b_1/((2^b_2)^2*(2^b_1)^2) + 5/27*b_2/((2^b_2)^2*(2^b_1)^2) + 5/27*b_0/((2^b_2)^2*(2^b_0)^2) + 5/27*b_2/((2^b_2)^2*(2^b_0)^2) + 5/27*b_0/((2^b_1)^2*(2^b_0)^2) + 5/27*b_1/((2^b_1)^2*(2^b_0)^2) - 4/81/((2^b_2)^2*(2^b_1)^2) + 1/27*b_0^2/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 2/27*b_0*b_1/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 2/27*b_0*b_2/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 1/27*b_1^2/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 2/27*b_1*b_2/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 1/27*b_2^2/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) - 4/81/((2^b_2)^2*(2^b_0)^2) - 4/81/((2^b_1)^2*(2^b_0)^2) - 11/27*b_0/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) - 11/27*b_1/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) - 11/27*b_2/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 64/81/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 35/81
sage: f
1/27*b_2^2/(2^b_2)^2 + 1/27*b_1^2/(2^b_1)^2 + 1/27*b_0^2/(2^b_0)^2 + 1/27*b_2/(2^b_2)^2 + 1/27*b_1/(2^b_1)^2 - 8/243/(2^b_2)^2 + 2/9/(2^b_2*2^b_1) + 2/9/(2^b_2*2^b_0) - 2/27*b_1*b_2/((2^b_2)^2*(2^b_1)^2) - 1/27*b_2^2/((2^b_2)^2*(2^b_1)^2) - 2/27*b_0*b_2/((2^b_2)^2*(2^b_0)^2) - 1/27*b_2^2/((2^b_2)^2*(2^b_0)^2) + 14/243/(2^b_1)^2 + 1/27*b_0/(2^b_0)^2 + 2/243/(2^b_2)^2 - 2/9/(2^b_2*2^b_1) - 2/9/(2^b_2*2^b_0) - 1/27*b_1^2/((2^b_2)^2*(2^b_1)^2) - 1/27*b_0^2/((2^b_2)^2*(2^b_0)^2) - 20/243/(2^b_1)^2 - 1/27*b_0^2/((2^b_1)^2*(2^b_0)^2) - 2/27*b_0*b_1/((2^b_1)^2*(2^b_0)^2) - 1/27*b_1^2/((2^b_1)^2*(2^b_0)^2) - 2/81/(2^b_0)^2 + 5/27*b_1/((2^b_2)^2*(2^b_1)^2) + 5/27*b_2/((2^b_2)^2*(2^b_1)^2) + 5/27*b_0/((2^b_2)^2*(2^b_0)^2) + 5/27*b_2/((2^b_2)^2*(2^b_0)^2) + 5/27*b_0/((2^b_1)^2*(2^b_0)^2) + 5/27*b_1/((2^b_1)^2*(2^b_0)^2) - 4/81/((2^b_2)^2*(2^b_1)^2) + 1/27*b_0^2/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 2/27*b_0*b_1/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 2/27*b_0*b_2/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 1/27*b_1^2/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 2/27*b_1*b_2/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 1/27*b_2^2/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) - 4/81/((2^b_2)^2*(2^b_0)^2) - 4/81/((2^b_1)^2*(2^b_0)^2) - 11/27*b_0/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) - 11/27*b_1/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) - 11/27*b_2/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 64/81/((2^b_2)^2*(2^b_1)^2*(2^b_0)^2) + 35/81
</pre><p>
The expression for f should get (a little bit) simplified.
</p>
<p>
For example, there are different summands where the only symbolic expressions used are (2^b_2)^-2 and they should get automatically gathered when pynac creates the object.
</p>
<p>
In fact calling expand() method on f gives you the right expression, but if things were working correctly you should not have to do this.
</p>
TicketMartin von GagernThu, 20 Jan 2011 08:10:21 GMT
https://trac.sagemath.org/ticket/9880#comment:16
https://trac.sagemath.org/ticket/9880#comment:16
<p>
Can someone experiencing this bug please provide a gdb backtrace? There is a (small) possibility that this issue here might be related to <a class="ext-link" href="https://github.com/cschwan/sage-on-gentoo/issues/issue/40/#issue/40/comment/679235"><span class="icon"></span>an issue</a> I encounter with sage on Gentoo Linux (with segfault in PyNaC as well), and I'd like to know for sure whether the error occurs inside the function <code>__cxxabiv1::__cxa_allocate_exception</code> or not.
</p>
TicketJean-Pierre FloriThu, 20 Jan 2011 10:51:02 GMT
https://trac.sagemath.org/ticket/9880#comment:17
https://trac.sagemath.org/ticket/9880#comment:17
<p>
The detailed description of the segfault mentionned here is available in the discussion mentionned in the ticket description.
</p>
<p>
I'll put it here so that everybody finds it:
</p>
<ul><li>the segfault happens in a call to std::sort() in a call to compare() because the ordering used by pynac is not a strict weak ordering. so it is kind of random. and I think youre bug is completely unrelated.
</li><li>the piece of code in the ticket description do not produce the segfault anymore since sage 4.6.1 (still present in 4.6.0), however the order was not changed, so it could still happen with other pieces of code, so here is a backtrace produce with a previous version of sage:
</li></ul><p>
#0 GiNaC::power::compare (this=0x449be20, other=...) at power.cpp:899 <br /> #1 0x00007fffd7a9cc18 in void <br /> std::__unguarded_linear_insert<__gnu_cxx::__normal_iterator<GiNaC::expair*, [[BR]] std::vector<GiNaC::expair, std::allocator<GiNaC::expair> > >, <br /> GiNaC::expair, <br /> GiNaC::expair_rest_is_less>(__gnu_cxx::__normal_iterator<GiNaC::expair*, [[BR]] std::vector<GiNaC::expair, std::allocator<GiNaC::expair> > >, <br /> GiNaC::expair, GiNaC::expair_rest_is_less) () <br /> from /home/jp/boulot/thèse/sage/sage-4.5.2/local/lib/ <br /> libpynac-0.2.so.0 <br /> #2 0x00007fffd7a9cefa in void <br /> std::__final_insertion_sort<__gnu_cxx::__normal_iterator<GiNaC::expair*, [[BR]] std::vector<GiNaC::expair, std::allocator<GiNaC::expair> > >, <br /> GiNaC::expair_rest_is_less>(__gnu_cxx::__normal_iterator<GiNaC::expair*, [[BR]] std::vector<GiNaC::expair, std::allocator<GiNaC::expair> > >, <br /> __gnu_cxx::__normal_iterator<GiNaC::expair*, [[BR]] std::vector<GiNaC::expair, std::allocator<GiNaC::expair> > >, <br /> GiNaC::expair_rest_is_less) () from /home/jp/boulot/thèse/sage/ <br /> sage-4.5.2/local/lib/libpynac-0.2.so.0 <br /> #3 0x00007fffd7a95657 in <br /> sort<__gnu_cxx::__normal_iterator<GiNaC::expair*, [[BR]] std::vector<GiNaC::expair, std::allocator<GiNaC::expair> > >, <br /> GiNaC::expair_rest_is_less> (this=<value optimized out>) at /usr/ <br /> include/c++/4.4/bits/stl_algo.h:5260 <br /> #4 GiNaC::expairseq::canonicalize (this=<value optimized out>) at <br /> expairseq.cpp:1177 <br /> #5 0x00007fffd7a994a4 in GiNaC::expairseq::construct_from_epvector <br /> (this=0x44a5070, v=<value optimized out>, <br /> do_index_renaming=<value optimized out>) at expairseq.cpp:1055 <br /> #6 0x00007fffd7a58685 in GiNaC::add::add (this=0x44a5070, vp=..., <br /> oc=<value optimized out>) at add.cpp:100 <br /> #7 0x00007fffd7a5871f in GiNaC::add::expand (this=0x449eb80, <br /> options=<value optimized out>) at add.cpp:669 <br /> #8 0x00007fffd7a9231d in GiNaC::ex::expand (this=<value optimized <br /> out>, options=3620337048) at ex.cpp:78 <br /> #9 0x00007fffd773e386 in <br /> __pyx_pf_4sage_8symbolic_10expression_10Expression_expand <br /> (__pyx_v_self=<value optimized out>, <br /> __pyx_args=0x601160, __pyx_kwds=0x0) at sage/symbolic/ <br /> expression.cpp:13604 <br /> #10 0x00007ffff7b107ca in call_function (f=0x44a4580, throwflag=<value <br /> optimized out>) at Python/ceval.c:3706 <br /> #11 !PyEval_EvalFrameEx (f=0x44a4580, throwflag=<value optimized out>) <br /> at Python/ceval.c:2389 <br /> #12 0x00007ffff7b124ad in !PyEval_EvalCodeEx (co=0x79c4c0, <br /> globals=<value optimized out>, locals=<value optimized out>, <br /> args=0x0, <br /> argcount=<value optimized out>, kws=0x0, kwcount=0, defs=0x0, <br /> defcount=0, closure=0x0) at Python/ceval.c:2968 <br /> #13 0x00007ffff7b12582 in !PyEval_EvalCode (co=0x449be20, <br /> globals=0x1135200007879, locals=0x7fffd7c9f598) at Python/ceval.c:522 <br /> #14 0x00007ffff7b3014c in run_mod ( <br /> . <br /> . <br /> . <br />
</p>
<p>
And valgrind output also: <br /> ==27910== Invalid read of size 8 <br /> ==27910== at 0x282C4BF1: void <br /> std::__unguarded_linear_insert<__gnu_cxx::__normal_iterator<GiNaC::expair*, [[BR]] std::vector<GiNaC::expair, std::allocator<GiNaC::expair> > >, <br /> GiNaC::expair, <br /> GiNaC::expair_rest_is_less>(__gnu_cxx::__normal_iterator<GiNaC::expair*, [[BR]] std::vector<GiNaC::expair, std::allocator<GiNaC::expair> > >, <br /> GiNaC::expair, GiNaC::expair_rest_is_less) (ptr.h:99) <br /> ==27910== by 0x282C4EF9: void <br /> std::__final_insertion_sort<__gnu_cxx::__normal_iterator<GiNaC::expair*, [[BR]] std::vector<GiNaC::expair, std::allocator<GiNaC::expair> > >, <br /> GiNaC::expair_rest_is_less>(__gnu_cxx::__normal_iterator<GiNaC::expair*, [[BR]] std::vector<GiNaC::expair, std::allocator<GiNaC::expair> > >, <br /> __gnu_cxx::__normal_iterator<GiNaC::expair*, [[BR]] std::vector<GiNaC::expair, std::allocator<GiNaC::expair> > >, <br /> GiNaC::expair_rest_is_less) (stl_algo.h:2161) <br /> ==27910== by 0x282BD656: GiNaC::expairseq::canonicalize() <br /> (stl_algo.h:5260) <br /> ==27910== by 0x282C14A3: <br /> GiNaC::expairseq::construct_from_epvector(std::vector<GiNaC::expair, [[BR]] std::allocator<GiNaC::expair> > const&, bool) (expairseq.cpp:1055) <br /> ==27910== by 0x28280684: <br /> GiNaC::add::add(std::auto_ptr<std::vector<GiNaC::expair, [[BR]] std::allocator<GiNaC::expair> > >, GiNaC::ex const&) (add.cpp:100) <br /> ==27910== by 0x2828071E: GiNaC::add::expand(unsigned int) const <br /> (add.cpp:669) <br /> ==27910== by 0x282BA31C: GiNaC::ex::expand(unsigned int) const <br /> (ex.cpp:78) <br /> ==27910== by 0x28811385: <br /> __pyx_pf_4sage_8symbolic_10expression_10Expression_expand(_object*, <br /> _object*, _object*) (expression.cpp:13604) <br /> ==27910== by 0x4F137C9: !PyEval_EvalFrameEx (ceval.c:3706) <br /> ==27910== by 0x4F154AC: !PyEval_EvalCodeEx (ceval.c:2968) <br /> ==27910== by 0x4F15581: !PyEval_EvalCode (ceval.c:522) <br /> ==27910== by 0x4F3314B: !PyRun_StringFlags (pythonrun.c:1335) <br /> ==27910== by 0x4F122DD: !PyEval_EvalFrameEx (ceval.c:4437) <br /> ==27910== by 0x4F154AC: !PyEval_EvalCodeEx (ceval.c:2968) <br /> ==27910== by 0x4E9BDDE: function_call (funcobject.c:524) <br /> ==27910== by 0x4E6FAA2: !PyObject_Call (abstract.c:2492) <br /> ==27910== by 0xD7E0981: <br /> __pyx_pf_4sage_9structure_11sage_object_load (sage_object.c:7304) <br /> ==27910== by 0x4F137C9: !PyEval_EvalFrameEx (ceval.c:3706) <br /> ==27910== by 0x4F154AC: !PyEval_EvalCodeEx (ceval.c:2968) <br /> ==27910== by 0x4F15581: !PyEval_EvalCode (ceval.c:522) <br /> ==27910== by 0x4F14853: !PyEval_EvalFrameEx (ceval.c:4401) <br /> ==27910== by 0x4F154AC: !PyEval_EvalCodeEx (ceval.c:2968) <br /> ==27910== by 0x4F13844: !PyEval_EvalFrameEx (ceval.c:3802) <br /> ==27910== by 0x4F154AC: !PyEval_EvalCodeEx (ceval.c:2968) <br /> ==27910== by 0x4F13844: !PyEval_EvalFrameEx (ceval.c:3802) <br /> ==27910== Address 0x97e4280 is 16 bytes before a block of size 416 <br /> alloc'd <br /> ==27910== at 0x4C24CCC: operator new(unsigned long) <br /> (vg_replace_malloc.c:220) <br /> ==27910== by 0x28286727: std::vector<GiNaC::expair, [[BR]] std::allocator<GiNaC::expair> >::reserve(unsigned long) <br /> (new_allocator.h:89) <br /> ==27910== by 0x282C0D4A: <br /> GiNaC::expairseq::make_flat(std::vector<GiNaC::expair, [[BR]] std::allocator<GiNaC::expair> > const&, bool) (expairseq.cpp:1138) <br /> ==27910== by 0x282C149B: <br /> GiNaC::expairseq::construct_from_epvector(std::vector<GiNaC::expair, [[BR]] std::allocator<GiNaC::expair> > const&, bool) (expairseq.cpp:1051) <br /> ==27910== by 0x28280684: <br /> GiNaC::add::add(std::auto_ptr<std::vector<GiNaC::expair, [[BR]] std::allocator<GiNaC::expair> > >, GiNaC::ex const&) (add.cpp:100) <br /> ==27910== by 0x2828071E: GiNaC::add::expand(unsigned int) const <br /> (add.cpp:669) <br /> ==27910== by 0x282BA31C: GiNaC::ex::expand(unsigned int) const <br /> (ex.cpp:78) <br /> ==27910== by 0x28811385: <br /> __pyx_pf_4sage_8symbolic_10expression_10Expression_expand(_object*, <br /> _object*, _object*) (expression.cpp:13604) <br /> ==27910== by 0x4F137C9: !PyEval_EvalFrameEx (ceval.c:3706) <br /> ==27910== by 0x4F154AC: !PyEval_EvalCodeEx (ceval.c:2968) <br /> ==27910== by 0x4F15581: !PyEval_EvalCode (ceval.c:522) <br /> ==27910== by 0x4F3314B: !PyRun_StringFlags (pythonrun.c:1335) <br /> ==27910== by 0x4F122DD: !PyEval_EvalFrameEx (ceval.c:4437) <br /> ==27910== by 0x4F154AC: !PyEval_EvalCodeEx (ceval.c:2968) <br /> ==27910== by 0x4E9BDDE: function_call (funcobject.c:524) <br /> ==27910== by 0x4E6FAA2: !PyObject_Call (abstract.c:2492) <br /> ==27910== by 0xD7E0981: <br /> __pyx_pf_4sage_9structure_11sage_object_load (sage_object.c:7304) <br /> ==27910== by 0x4F137C9: !PyEval_EvalFrameEx (ceval.c:3706) <br /> ==27910== by 0x4F154AC: !PyEval_EvalCodeEx (ceval.c:2968) <br /> ==27910== by 0x4F15581: !PyEval_EvalCode (ceval.c:522) <br /> ==27910== by 0x4F14853: !PyEval_EvalFrameEx (ceval.c:4401) <br /> ==27910== by 0x4F154AC: !PyEval_EvalCodeEx (ceval.c:2968) <br /> ==27910== by 0x4F13844: !PyEval_EvalFrameEx (ceval.c:3802) <br /> ==27910== by 0x4F154AC: !PyEval_EvalCodeEx (ceval.c:2968) <br /> ==27910== by 0x4F13844: !PyEval_EvalFrameEx (ceval.c:3802)
</p>
TicketVolker BraunMon, 28 Feb 2011 14:47:22 GMT
https://trac.sagemath.org/ticket/9880#comment:18
https://trac.sagemath.org/ticket/9880#comment:18
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:17" title="Comment 17">jpflori</a>:
</p>
<blockquote class="citation">
<ul><li>the segfault happens in a call to std::sort() in a call to compare() because the ordering used by pynac is not a strict weak ordering. so it is kind of random.
</li></ul></blockquote>
<p>
<code>std::sort</code> with anything but a strict weak ordering is a receipe for disaster. It is expected to crash, and it does in the example. So you are saying that we must never use GiNaC internal order for sorting.
</p>
<p>
I take it that <code>expair_is_greater_degrevlex</code> does implement a strict weak ordering, so it is safe to use with <code>std::sort</code>?
</p>
<p>
Is the patch in this ticket still relevant or has this been fixed elsewhere?
</p>
TicketJean-Pierre FloriMon, 28 Feb 2011 17:22:19 GMT
https://trac.sagemath.org/ticket/9880#comment:19
https://trac.sagemath.org/ticket/9880#comment:19
<p>
The problem is not the GiNaC original internal order (even if <a class="ext-link" href="http://www.ginac.de/reference/structGiNaC_1_1expair__rest__is__less.html"><span class="icon"></span>http://www.ginac.de/reference/structGiNaC_1_1expair__rest__is__less.html</a> states it is not a SWO... at least using it makes the bug disappear and inconsistencies in automatic simplifications disappear; if it does not work, i guess it should be fixed by GiNaC team) but the modification of that order used in pynac (and so in Sage).
</p>
<p>
AFAIK the two patches here are still necessary, the segfault disappeared (!?!) but i still get some problems with automatic simplification as shown in <a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/9880#comment:15"><span class="icon"></span>http://trac.sagemath.org/sage_trac/ticket/9880#comment:15</a>.
</p>
<ul><li>the first patch (by Burcin) reverts the original GiNaC ordering for internal use (e.g. to be used in call to std::sort) in pynac and use the modified order only for printing (it should also be uesd at some point for operands access and so on).
</li></ul><ul><li>the second one (by me) should be applied on top of the first one and tries to polish the order used for printing so that is looks more like degrevlex.
</li></ul><p>
It should not be much more difficult to implement other orders for printing (and operands access).
</p>
TicketVolker BraunMon, 28 Feb 2011 17:41:16 GMT
https://trac.sagemath.org/ticket/9880#comment:20
https://trac.sagemath.org/ticket/9880#comment:20
<p>
I have a suspicion that gcc's <code>std::sort</code> implementation changed which hides the bug. But just because its hidden doesn't mean that its still there. I'm currently trying to install some old versions to test.
</p>
TicketVolker BraunTue, 01 Mar 2011 13:19:47 GMT
https://trac.sagemath.org/ticket/9880#comment:21
https://trac.sagemath.org/ticket/9880#comment:21
<p>
I don't understand the GiNaC documentation that you referred to (<a class="ext-link" href="http://www.ginac.de/reference/structGiNaC_1_1expair__rest__is__less.html"><span class="icon"></span>http://www.ginac.de/reference/structGiNaC_1_1expair__rest__is__less.html</a>). They state it is not a SWO, and their example is that neither 3*x<2*x nor 2*x<3*x. But thats perfectly fine in a SWO, you can have incomparable elements. The only constraint on incomparable elements is transitivity, that is, if A and B are incomparable and B and C are incomparable then A and C are also incomparable. Do you understand why its not a SWO?
</p>
TicketBurcin ErocalTue, 01 Mar 2011 13:27:58 GMT
https://trac.sagemath.org/ticket/9880#comment:22
https://trac.sagemath.org/ticket/9880#comment:22
<p>
<a class="closed ticket" href="https://trac.sagemath.org/ticket/10833" title="#10833: defect: Unhandled SIGSEGV on large expand of iterated polynomial (closed: duplicate)">#10833</a> was a duplicate of this. Here is the example from that ticket:
</p>
<pre class="wiki">phi(x) = x^2 + c
def iterkate(n):
pol = x
for i in range(1,n):
pol = phi(pol)
return pol
g = expand(iterkate(7))
</pre>
TicketJean-Pierre FloriTue, 01 Mar 2011 13:32:38 GMT
https://trac.sagemath.org/ticket/9880#comment:23
https://trac.sagemath.org/ticket/9880#comment:23
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:21" title="Comment 21">vbraun</a>:
</p>
<blockquote class="citation">
<p>
I don't understand the GiNaC documentation that you referred to (<a class="ext-link" href="http://www.ginac.de/reference/structGiNaC_1_1expair__rest__is__less.html"><span class="icon"></span>http://www.ginac.de/reference/structGiNaC_1_1expair__rest__is__less.html</a>). They state it is not a SWO, and their example is that neither 3*x<2*x nor 2*x<3*x. But thats perfectly fine in a SWO, you can have incomparable elements. The only constraint on incomparable elements is transitivity, that is, if A and B are incomparable and B and C are incomparable then A and C are also incomparable. Do you understand why its not a SWO?
</p>
</blockquote>
<p>
I have no idea why GiNaC order is not an SWO, my point was just to link that page where the GiNaC devs state it is not one.
</p>
<p>
However I never had problems with it so far, so maybe it is a SWO and the statement in GiNaC doc is just wrong.
</p>
<p>
We should post on GiNaC mailing list to get more info on that one. Maybe Burcin knows also.
</p>
<p>
What is definitely sure is that the modified order used in pynac is not correct. Because of it the result of a call to std::sort is flawed. This can be not too harmful (e.g. automatic simplification does not occur because terms which should be adjacent in the internal structure are not) but can also lead to segfaults (even if that dramatic side effect seems to depend on something mysterious, potentially on the gcc version used as you stated).
</p>
<p>
So at least for me, using the original GiNaC order solves many problems.
</p>
TicketVolker BraunTue, 01 Mar 2011 13:35:00 GMT
https://trac.sagemath.org/ticket/9880#comment:24
https://trac.sagemath.org/ticket/9880#comment:24
<p>
Burcin: If you are working on pynac, can you add some private function/method that exposes the GiNaC order to Sage in addition to the Sage (printing) order? Then we can add some randomized testing to make sure that both are strict weak orders.
</p>
TicketJean-Pierre FloriTue, 01 Mar 2011 14:43:06 GMT
https://trac.sagemath.org/ticket/9880#comment:25
https://trac.sagemath.org/ticket/9880#comment:25
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:24" title="Comment 24">vbraun</a>:
</p>
<blockquote class="citation">
<p>
Burcin: If you are working on pynac, can you add some private function/method that exposes the GiNaC order to Sage in addition to the Sage (printing) order? Then we can add some randomized testing to make sure that both are strict weak orders.
</p>
</blockquote>
<p>
Calling the _cmp_ method of an expression should give you access to pynac internal ordering (i.e. with the patched spkg, it is GiNaC original ordering; with the old one you would get the modified order currently used by pynac).
</p>
<p>
For example (with the new spkg):
</p>
<pre class="wiki">sage: var('a b')
(a, b)
sage: x._cmp_(a)
1
sage: a._cmp_(b)
1
sage: a+b
a + b
sage: x+a
a + x
</pre><p>
In GiNaC order, vars are ordered according to creation order iirc. So here we have x (automatically created) > a > b.
</p>
<p>
For printing, we use lexicographic order a > b > x and print bigger terms first.
</p>
<p>
To check it the only current way is to look at what gets printed, the internal ordering is used for everything else.
</p>
<p>
I'll try to post a minimal patch to have access to that order in sage.
</p>
TicketJean-Pierre FloriTue, 01 Mar 2011 16:23:02 GMT
https://trac.sagemath.org/ticket/9880#comment:26
https://trac.sagemath.org/ticket/9880#comment:26
<p>
Here you go:
</p>
<ol><li>Install the new spkg available at <a class="ext-link" href="http://perso.telecom-paristech.fr/~flori/pynac-0.2.1-order.spkg"><span class="icon"></span>http://perso.telecom-paristech.fr/~flori/pynac-0.2.1-order.spkg</a>
</li><li>Apply pynac-order.patch
</li><li>Rebuild Sage
</li><li>Expession elements have now two new methods _cmp_add and _cmp_mul giving access to the order used for printing.
</li></ol><p>
_cmp_add is (more or less) the function used to order elements in a sum and _cmp_mul in a product. Their behavior is not exactly the same (don't remember why, it's been a while).
</p>
TicketJean-Pierre FloriTue, 01 Mar 2011 16:23:43 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>pynac-order.patch</em>
</li>
</ul>
<p>
Access to comparison function used for printing.
</p>
TicketVolker BraunWed, 02 Mar 2011 10:52:15 GMT
https://trac.sagemath.org/ticket/9880#comment:27
https://trac.sagemath.org/ticket/9880#comment:27
<p>
Jean-Pierre: I think paristech.fr has some issues at the moment, can you upload it somewhere else?
</p>
TicketJean-Pierre FloriWed, 02 Mar 2011 11:23:17 GMTowner changed
https://trac.sagemath.org/ticket/9880#comment:28
https://trac.sagemath.org/ticket/9880#comment:28
<ul>
<li><strong>owner</strong>
changed from <em>Burcin Erocal</em> to <em>Jean-Pierre Flori</em>
</li>
</ul>
<p>
You can try accessing the same server at <a class="ext-link" href="http://www.enst.fr/~flori/"><span class="icon"></span>http://www.enst.fr/~flori/</a> or another one form the school at <a class="ext-link" href="http://www.infres.enst.fr/~flori/"><span class="icon"></span>http://www.infres.enst.fr/~flori/</a> (same content, other server).
</p>
<p>
I've put also the package at <a class="ext-link" href="http://viedethesarde.free.fr/sage/"><span class="icon"></span>http://viedethesarde.free.fr/sage/</a>
</p>
<p>
You should get the file pynac-0.2.1-patched.spkg (not the -order one I deleted anyway).
</p>
<p>
It is quite the same as the one you would get by applying the patches of the ticket to pyanc-0.2.1.
</p>
TicketJean-Pierre FloriWed, 02 Mar 2011 11:24:24 GMTowner changed
https://trac.sagemath.org/ticket/9880#comment:29
https://trac.sagemath.org/ticket/9880#comment:29
<ul>
<li><strong>owner</strong>
changed from <em>Jean-Pierre Flori</em> to <em>Burcin Erocal</em>
</li>
</ul>
TicketVolker BraunWed, 02 Mar 2011 12:54:30 GMT
https://trac.sagemath.org/ticket/9880#comment:30
https://trac.sagemath.org/ticket/9880#comment:30
<p>
For the record, both the snippet in the ticket description and the <code>iterkate()</code> example reliably crash Sage-4.6.1 on Ubuntu-9.04.
</p>
<p>
The updated pynac-0.2.1-patched.spkg fixes this problem, and sage no longer crashes.
</p>
TicketVolker BraunWed, 02 Mar 2011 14:19:47 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>strict_weak_order.py</em>
</li>
</ul>
<p>
Randomized testing of orders
</p>
TicketVolker BraunWed, 02 Mar 2011 14:23:45 GMT
https://trac.sagemath.org/ticket/9880#comment:31
https://trac.sagemath.org/ticket/9880#comment:31
<p>
With the attached script I find some examples in <code>cmp_add</code> where <code>a<b<c<a</code>. This violates SWO:
</p>
<pre class="wiki">sage: attach strict_weak_order.py
sage: test_symbolic_expression_order(10000)
---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
/home/vbraun/Sage/Order/<ipython console> in <module>()
/home/vbraun/Sage/Order/strict_weak_order.py in test_symbolic_expression_order(repetitions)
113 c = make_random_expr()
114 assert_strict_weak_order(a, b, c, lambda x,y: x._cmp_(y))
--> 115 assert_strict_weak_order(a, b, c, lambda x,y: x._cmp_add(y))
116 assert_strict_weak_order(a, b, c, lambda x,y: x._cmp_mul(y))
117
/home/vbraun/Sage/Order/strict_weak_order.py in assert_strict_weak_order(a, b, c, cmp_func)
63
64 for i,j,k in Permutations([0,1,2]): # transitivity
---> 65 if cmp[i,j] and cmp[j,k] and not cmp[i,k]: raise ValueError, msg
66
67 def incomparable(i,j):
ValueError: The binary relation failed to be a strict weak order on the elements
a = 2*v10*(v5 - 2)*v7 + (-(v8 + e)*(-v8 + pi) + v6)*(v9*e - 2) - v2 - v3 - v5 - v9
b = -(v3*v8 - 51*v2 - 105)*(v2 + 5)*(v3 - 1) + 3*v6*v9
c = -v1*v6*brun + v6*v9*pi + 3*(7*(v5 + 1)*v7 - 3)*(-(v3 - 1)*e + v4) - (v5 - 4)*(v9 + 8) - v3 - 3
[0 0 1]
[1 0 0]
[0 1 0]
</pre>
TicketJean-Pierre FloriWed, 02 Mar 2011 14:26:22 GMT
https://trac.sagemath.org/ticket/9880#comment:32
https://trac.sagemath.org/ticket/9880#comment:32
<p>
Not sure it is related, but i found the ordering of functions is currently random for functions having the same name.
</p>
<p>
I'm currently fixing this.
</p>
TicketJean-Pierre FloriWed, 02 Mar 2011 18:35:07 GMT
https://trac.sagemath.org/ticket/9880#comment:33
https://trac.sagemath.org/ticket/9880#comment:33
<p>
Ok that was unrelated.
</p>
<p>
It happened because some types were not handled by the new ordering and compared type ids directly which screw things up.
</p>
<p>
I launched a big test with your program and we'll see if it has reported any problems tomorrow.
</p>
TicketJean-Pierre FloriThu, 03 Mar 2011 15:18:24 GMT
https://trac.sagemath.org/ticket/9880#comment:34
https://trac.sagemath.org/ticket/9880#comment:34
<p>
I uploaded a new version of the spkg that you can get at:
</p>
<p>
<a class="ext-link" href="http://www.infres.enst.fr/~flori/pynac-0.2.1-patched.spkg"><span class="icon"></span>http://www.infres.enst.fr/~flori/pynac-0.2.1-patched.spkg</a>
</p>
<p>
or:
</p>
<p>
<a class="ext-link" href="http://perso.telecom-paristech.fr/~flori/pynac-0.2.1-patched.spkg"><span class="icon"></span>http://perso.telecom-paristech.fr/~flori/pynac-0.2.1-patched.spkg</a>
</p>
<p>
I'm still running tests using test_symbolic_expression_order but did not get new errors.
</p>
TicketJean-Pierre FloriFri, 15 Apr 2011 09:12:34 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-pynac_order_jp_new-p2.patch</em>
</li>
</ul>
<p>
New version of patch, apply after burcin original one to build updated spkg
</p>
TicketVolker BraunWed, 04 May 2011 14:59:57 GMTstatus changed
https://trac.sagemath.org/ticket/9880#comment:35
https://trac.sagemath.org/ticket/9880#comment:35
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Jean-Pierre: Are you now finished with running your tests? Can you bring the spkg into a reviewable state? At the very least it should be called pynac-0.2.1.p0.spkg and have a SPKG.txt entry.
</p>
TicketBurcin ErocalWed, 04 May 2011 15:22:08 GMT
https://trac.sagemath.org/ticket/9880#comment:36
https://trac.sagemath.org/ticket/9880#comment:36
<p>
Please excuse my ignorance, especially since I promised to make a pynac release which includes this patch ages ago, but I have a few questions and not enough time to test this and find out the answers on my own:
</p>
<ul><li>doesn't this require a patch to the Sage library at least to fix doctests? Is the new printing order exactly the same as the old (inconsistent) one?
</li></ul><ul><li>Don't we also need to modify the operand access functions (at least the one in Sage - not the .op() function of ginac) to return the operands in the sorted order, not the stored (somewhat random) order?
</li></ul><p>
BTW, Jean-Pierre, I wouldn't mind at all if you want to cut the new pynac release yourself. I can provide instructions on how to do this.
</p>
TicketJean-Pierre FloriWed, 04 May 2011 15:35:45 GMT
https://trac.sagemath.org/ticket/9880#comment:37
https://trac.sagemath.org/ticket/9880#comment:37
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:36" title="Comment 36">burcin</a>:
</p>
<blockquote class="citation">
<ul><li>doesn't this require a patch to the Sage library at least to fix doctests? Is the new printing order exactly the same as the old (inconsistent) one?
</li></ul></blockquote>
<p>
I don't think this order and the old one will coincide and a lot of doctests will have to be fixed if we use it.
I'll try to have a look at all of this soon, I must admit I did not touch that code since my last post here.
</p>
<blockquote class="citation">
<ul><li>Don't we also need to modify the operand access functions (at least the one in Sage - not the .op() function of ginac) to return the operands in the sorted order, not the stored (somewhat random) order?
</li></ul></blockquote>
<p>
I think you are right.
I did not test it but it should return unexpected values with the new code.
I'll include that when the new order seems consistent.
</p>
<blockquote class="citation">
<p>
BTW, Jean-Pierre, I wouldn't mind at all if you want to cut the new pynac release yourself. I can provide instructions on how to do this.
</p>
</blockquote>
<p>
Please go ahead, I'll find the time to do it.
</p>
<p>
I'm currently rebuilding everything and running some tests, but IIRC the piece of code Volker provided did not raise errors anymore.
Of course there could be other inconsistencies here and there.
</p>
TicketJean-Pierre FloriThu, 05 May 2011 08:17:20 GMT
https://trac.sagemath.org/ticket/9880#comment:38
https://trac.sagemath.org/ticket/9880#comment:38
<p>
I did not get inconsistencies using Volker code yet. I'll package a candidate updated spkg today so that someone can have a look at the code update in pynac.
</p>
<p>
I'll do a proper pynac release when the code is positively reviewed and Burcin tells me how to.
</p>
<p>
Here is the list of doctests failure with the new spkg:
</p>
<pre class="wiki">----------------------------------------------------------------------
The following tests failed:
sage -t -force_lib devel/sage/doc/en/constructions/polynomials.rst # 1 doctests failed
sage -t -force_lib devel/sage/doc/en/constructions/calculus.rst # 7 doctests failed
sage -t -force_lib devel/sage/doc/en/tutorial/tour_algebra.rst # 3 doctests failed
sage -t -force_lib devel/sage/doc/en/tutorial/introduction.rst # 2 doctests failed
sage -t -force_lib devel/sage/doc/en/a_tour_of_sage/index.rst # 1 doctests failed
sage -t -force_lib devel/sage/doc/en/bordeaux_2008/nf_introduction.rst # 1 doctests failed
sage -t -force_lib devel/sage/doc/fr/tutorial/tour_algebra.rst # 3 doctests failed
sage -t -force_lib devel/sage/doc/fr/tutorial/introduction.rst # 2 doctests failed
sage -t -force_lib devel/sage/doc/fr/a_tour_of_sage/index.rst # 1 doctests failed
sage -t -force_lib devel/sage/sage/modules/vector_callable_symbolic_dense.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/modules/free_module_element.pyx # 3 doctests failed
sage -t -force_lib devel/sage/sage/interfaces/maxima_abstract.py # 3 doctests failed
sage -t -force_lib devel/sage/sage/interfaces/maxima_lib.py # 2 doctests failed
sage -t -force_lib devel/sage/sage/numerical/optimize.py # 2 doctests failed
sage -t -force_lib devel/sage/sage/tensor/differential_form_element.py # 2 doctests failed
sage -t -force_lib devel/sage/sage/rings/integer.pyx # 2 doctests failed
sage -t -force_lib devel/sage/sage/rings/qqbar.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/rings/power_series_ring.py # 2 doctests failed
sage -t -force_lib devel/sage/sage/rings/number_field/number_field_element.pyx # 1 doctests failed
sage -t -force_lib devel/sage/sage/rings/polynomial/polynomial_element.pyx # 1 doctests failed
sage -t -force_lib devel/sage/sage/gsl/dft.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/calculus/functional.py # 12 doctests failed
sage -t -force_lib devel/sage/sage/calculus/tests.py # 16 doctests failed
sage -t -force_lib devel/sage/sage/calculus/desolvers.py # 14 doctests failed
sage -t -force_lib devel/sage/sage/calculus/calculus.py # 19 doctests failed
sage -t -force_lib devel/sage/sage/calculus/test_sympy.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/calculus/functions.py # 3 doctests failed
sage -t -force_lib devel/sage/sage/calculus/wester.py # 10 doctests failed
sage -t -force_lib devel/sage/sage/calculus/var.pyx # 2 doctests failed
sage -t -force_lib devel/sage/sage/categories/classical_crystals.py # 2 doctests failed
sage -t -force_lib devel/sage/sage/combinat/perfect_matching.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/combinat/partition.py # 2 doctests failed
sage -t -force_lib devel/sage/sage/combinat/sf/ns_macdonald.py # 2 doctests failed
sage -t -force_lib devel/sage/sage/ext/fast_callable.pyx # 3 doctests failed
sage -t -force_lib devel/sage/sage/schemes/elliptic_curves/ell_generic.py # 2 doctests failed
sage -t -force_lib devel/sage/sage/stats/basic_stats.py # 7 doctests failed
sage -t -force_lib devel/sage/sage/functions/log.py # 2 doctests failed
sage -t -force_lib devel/sage/sage/functions/hyperbolic.py # 3 doctests failed
sage -t -force_lib devel/sage/sage/functions/special.py # 4 doctests failed
sage -t -force_lib devel/sage/sage/functions/orthogonal_polys.py # 4 doctests failed
sage -t -force_lib devel/sage/sage/functions/trig.py # 5 doctests failed
sage -t -force_lib devel/sage/sage/functions/wigner.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/functions/other.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/functions/piecewise.py # 12 doctests failed
sage -t -force_lib devel/sage/sage/matrix/matrix_symbolic_dense.pyx # 17 doctests failed
sage -t -force_lib devel/sage/sage/matrix/matrix2.pyx # 3 doctests failed
sage -t -force_lib devel/sage/sage/symbolic/function_factory.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/symbolic/maxima_wrapper.py # 14 doctests failed
sage -t -force_lib devel/sage/sage/symbolic/expression_conversions.py # 7 doctests failed
sage -t -force_lib devel/sage/sage/symbolic/ring.pyx # 3 doctests failed
sage -t -force_lib devel/sage/sage/symbolic/constants.py # 4 doctests failed
sage -t -force_lib devel/sage/sage/symbolic/random_tests.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/symbolic/relation.py # 13 doctests failed
sage -t -force_lib devel/sage/sage/symbolic/function.pyx # 2 doctests failed
sage -t -force_lib devel/sage/sage/symbolic/callable.py # 2 doctests failed
sage -t -force_lib devel/sage/sage/symbolic/assumptions.py # 11 doctests failed
sage -t -force_lib devel/sage/sage/symbolic/integration/integral.py # 9 doctests failed
sage -t -force_lib devel/sage/sage/symbolic/expression.pyx # 162 doctests failed
sage -t -force_lib devel/sage/sage/plot/plot3d/plot3d.py # 6 doctests failed
sage -t -force_lib devel/sage/sage/misc/preparser.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/misc/functional.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/graphs/generic_graph.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/misc/prandom.py # 1 doctests failed
sage -t -force_lib devel/sage/sage/misc/parser.pyx # 2 doctests failed
----------------------------------------------------------------------
</pre><p>
I hope that all of them are trivial...
</p>
<p>
One question for Burcin: when you speak of operand access, are you thinking of <a class="closed ticket" href="https://trac.sagemath.org/ticket/9989" title="#9989: enhancement: easier access to operands of a symbolic expression (closed: fixed)">#9989</a> or am I missing something in the current Sage code ?
</p>
TicketJean-Pierre FloriThu, 05 May 2011 09:15:57 GMT
https://trac.sagemath.org/ticket/9880#comment:39
https://trac.sagemath.org/ticket/9880#comment:39
<p>
A new spkg is available at <a class="ext-link" href="http://perso.telecom-paristech.fr/~flori/sage/pynac-0.2.1.p0.spkg"><span class="icon"></span>http://perso.telecom-paristech.fr/~flori/sage/pynac-0.2.1.p0.spkg</a> .
</p>
<p>
I just updated SPKG.txt and commited the changeset in comparison with pynac-0.2.1-patched.spkg .
</p>
<p>
The changes i pynac code from 0.2.1 are the result of applying:
</p>
<ol><li><a class="ext-link" href="http://trac.sagemath.org/sage_trac/attachment/ticket/9880/trac9880_pynac_order_burcin_original.patch"><span class="icon"></span>trac9880_pynac_order_burcin_original.patch</a>
</li><li><a class="ext-link" href="http://trac.sagemath.org/sage_trac/attachment/ticket/9880/trac_9880-pynac_order_jp_new-p2.patch"><span class="icon"></span>trac_9880-pynac_order_jp_new-p2.patch</a>
</li></ol><p>
If you want to have access to the comparison functions used by the printing order (to stress pynac with Volker code <a class="ext-link" href="http://trac.sagemath.org/sage_trac/attachment/ticket/9880/strict_weak_order.py"><span class="icon"></span>strict_weak_order.py</a> for example) you must also apply the patch access_order.patch .
</p>
TicketBurcin ErocalSat, 07 May 2011 14:44:59 GMT
https://trac.sagemath.org/ticket/9880#comment:40
https://trac.sagemath.org/ticket/9880#comment:40
<p>
I don't want to delay this any further, but if this is merged all patches touching anything symbolic will need to be rebased. I suggest we try to review and merge any feasible symbolics tickets for the next release. Then this gets merged for 4.8 with big warnings, since users would also need to change doctests in any private code they might have.
</p>
<p>
I can actually spare time for such an effort now. I will see what I can do with the needs review/work tickets on trac starting tomorrow.
</p>
<p>
BTW, we should also include Volker's test script. Perhaps as a part of <code>sage/symbolics/random_tests.py</code>.
</p>
<p>
Jean-Pierre: Yes, by operand access I mean the <code>.operands()</code> and the recent <code>.op</code> from <a class="closed ticket" href="https://trac.sagemath.org/ticket/9989" title="#9989: enhancement: easier access to operands of a symbolic expression (closed: fixed)">#9989</a>.
</p>
<p>
Making a pynac release is just a matter of editing <code>configure.ac</code>, setting the version number according to the comments there and running <code>autoreconf</code> afterwards. I am not sure any more if the next pynac release should contain this. This can be pynac 0.3.0, and I will put in the patches in my queue corresponding to some of the tickets waiting on trac for 0.2.2.
</p>
<p>
Note that I imported the pynac-0.2.1 repository into bitbucket:
</p>
<p>
<a class="ext-link" href="https://bitbucket.org/burcin/pynac"><span class="icon"></span>https://bitbucket.org/burcin/pynac</a>
</p>
<p>
I hope to use the facilities there to streamline the development process.
</p>
TicketVolker BraunWed, 15 Jun 2011 00:37:33 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880_fix_import.patch</em>
</li>
</ul>
<p>
Initial patch
</p>
TicketVolker BraunWed, 15 Jun 2011 00:37:47 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880_pynac_order.patch</em>
</li>
</ul>
<p>
Initial patch
</p>
TicketVolker BraunWed, 15 Jun 2011 00:37:56 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880_randomized_testing.patch</em>
</li>
</ul>
<p>
Initial patch
</p>
TicketVolker BraunWed, 15 Jun 2011 00:40:46 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:41
https://trac.sagemath.org/ticket/9880#comment:41
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=41">diff</a>)
</li>
</ul>
TicketBurcin ErocalWed, 15 Jun 2011 03:50:52 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:42
https://trac.sagemath.org/ticket/9880#comment:42
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=42">diff</a>)
</li>
</ul>
<p>
I changed the structure of the order classes a little. New patches are available in my pynac queue:
</p>
<p>
<a class="ext-link" href="https://bitbucket.org/burcin/pynac-patches/src"><span class="icon"></span>https://bitbucket.org/burcin/pynac-patches/src</a>
</p>
<p>
<a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9880/trac_9880_pynac_order.take2.patch" title="Attachment 'trac_9880_pynac_order.take2.patch' in Ticket #9880">attachment:trac_9880_pynac_order.take2.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9880/trac_9880_pynac_order.take2.patch" title="Download"></a> should be applied to access these functions from Sage.
</p>
TicketBurcin ErocalThu, 16 Jun 2011 19:48:23 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880_pynac_order.take2.patch</em>
</li>
</ul>
<p>
updated JP's patch for sage library to work with the new order patches
</p>
TicketBurcin ErocalThu, 16 Jun 2011 19:48:35 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-stable_operands.patch</em>
</li>
</ul>
TicketBurcin ErocalThu, 16 Jun 2011 19:50:34 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:43
https://trac.sagemath.org/ticket/9880#comment:43
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=43">diff</a>)
</li>
</ul>
TicketKarl-Dieter CrismanThu, 16 Jun 2011 19:58:42 GMT
https://trac.sagemath.org/ticket/9880#comment:44
https://trac.sagemath.org/ticket/9880#comment:44
<p>
What remains for 'needs review' so one could review it?
</p>
TicketBurcin ErocalThu, 16 Jun 2011 20:10:34 GMT
https://trac.sagemath.org/ticket/9880#comment:45
https://trac.sagemath.org/ticket/9880#comment:45
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:44" title="Comment 44">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
What remains for 'needs review' so one could review it?
</p>
</blockquote>
<p>
Volker has changes to pynac to fix simplification of expressions involving infinity when we can't assume anything about the internal ordering.
</p>
<p>
I am trying to fix printing of <code>mul</code> objects where we get <code>-</code> right after a parenthesis, such as <code>-(4*cos(x)^2 - 1)*sin(x)/(-4*cos(x)^3 + 3*cos(x))</code>.
</p>
<p>
It might be good to update my <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9880/trac_9880-stable_operands.patch" title="Attachment 'trac_9880-stable_operands.patch' in Ticket #9880">trac_9880-stable_operands.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9880/trac_9880-stable_operands.patch" title="Download"></a> to make it optional to use the printing order.
</p>
<p>
Then a lot of doctests need to get fixed. We also need to document that <code>.find()</code>, <code>.match()</code> and friends will not return results in a canonical order any more and that expressions constructed with <code>hold =True</code> do not print in the order provided by the user.
</p>
TicketVolker BraunFri, 17 Jun 2011 06:22:20 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:46
https://trac.sagemath.org/ticket/9880#comment:46
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=46">diff</a>)
</li>
</ul>
TicketVolker BraunFri, 17 Jun 2011 19:06:30 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_comparison.patch</em>
</li>
</ul>
<p>
Initial patch
</p>
TicketVolker BraunFri, 17 Jun 2011 19:08:49 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:47
https://trac.sagemath.org/ticket/9880#comment:47
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=47">diff</a>)
</li>
</ul>
<p>
Patch fixes the segfault
</p>
<pre class="wiki">sage: x._cmp_add(1)
</pre>
TicketVolker BraunFri, 17 Jun 2011 19:37:05 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880_pynac_infinities.patch</em>
</li>
</ul>
<p>
Fixed commit message.
</p>
TicketVolker BraunSat, 18 Jun 2011 03:11:59 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_doctests_symbolic.patch</em>
</li>
</ul>
<p>
Initial patch
</p>
TicketVolker BraunSat, 18 Jun 2011 03:14:09 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:48
https://trac.sagemath.org/ticket/9880#comment:48
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=48">diff</a>)
</li>
</ul>
<p>
The <code>trac_9880-fix_doctests_symbolic.patch</code> fixes the bulk of the doctests in <code>sage.symbolic.expression.pyx</code> and <code>sage.symbolic.random_tests.py</code>. There are a few remaining that depend on a random sign in the print output that Burcin is fixing right now.
</p>
TicketVolker BraunTue, 21 Jun 2011 18:24:12 GMT
https://trac.sagemath.org/ticket/9880#comment:49
https://trac.sagemath.org/ticket/9880#comment:49
<p>
tdupu on <code>#sagemath</code> reported the following on sage-4.7.1.alpha2:
</p>
<pre class="wiki">sage: ff = 117306*x^2 + x
sage: gg = x + 7^3*x^2
sage: R.<x> = PolynomialRing(ZZ.quotient(7^6),1)
sage: ggred = R(gg)
sage: print expand(ff(x=ggred))
0*x^4 + 0*x^3
sage: print expand(ggred(x=ff))
x
</pre><p>
This is also fixed in the updated pynac and now prints <code>x</code> both times.
</p>
TicketBurcin ErocalTue, 28 Jun 2011 13:57:17 GMT
https://trac.sagemath.org/ticket/9880#comment:50
https://trac.sagemath.org/ticket/9880#comment:50
<p>
I updated my <a class="ext-link" href="https://bitbucket.org/burcin/pynac-patches"><span class="icon"></span>pynac queue</a> with a new patch that fixes the random sign Volker mentioned in <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:48" title="Comment 48">comment:48</a>. It turns out that the issue is bigger. The result printed in some cases might be wrong, since the internal structure of an <code>add</code> object is modified when <code>seq_sorted</code> is already stored. I added an assert statement in <code>{add,mul}::get_sorted_seq()</code> to test consistency. With the <code>minus.patch</code> from my queue applied, I still get crashes while testing <code>sage/calculus/tests.py</code>. So this needs more work.
</p>
TicketJean-Pierre FloriTue, 28 Jun 2011 14:09:20 GMT
https://trac.sagemath.org/ticket/9880#comment:51
https://trac.sagemath.org/ticket/9880#comment:51
<p>
Thanks for working on this.
</p>
<p>
I'll try to build your updated version and have a look at the crashes this week.
</p>
TicketJean-Pierre FloriMon, 19 Sep 2011 13:46:22 GMT
https://trac.sagemath.org/ticket/9880#comment:52
https://trac.sagemath.org/ticket/9880#comment:52
<p>
So I'm finally having a look at that.
</p>
<p>
Here are some examples producing the bug or not:
</p>
<pre class="wiki">sage: var('t')
t
sage: (x-t)^3
(-t + x)^3
sage: (-t+x)^3
(-t + x)^3
sage: (-x+t)^3
Program received signal...
sage: (t-x)^3
Program received signal...
sage: var('z')
z
sage: (-x+z)^3
-(x - z)^3
sage: (x-z)^3
(x - z )^3
sage: (-z+x)^3
(x - z)^3
sage: (z-x)^3
-(x - z)^3
sage: var('y')
y
sage: (y-t)^3
-(t - y)^3
sage: (t-y)^3
(t - y)^3
sage: (-y+z)^3
(-y + z)^3
sage: (y-z)^3
Program received signal ...
</pre>
TicketJean-Pierre FloriMon, 19 Sep 2011 15:51:43 GMT
https://trac.sagemath.org/ticket/9880#comment:53
https://trac.sagemath.org/ticket/9880#comment:53
<p>
I guess the problem lies in line 1528 of mul.cpp.
</p>
<p>
When building the new mul object if there is a leading minus, I think it enters an infinite loop.
</p>
<p>
And isn't there a better way to check for leading minus than to emulate printing (even at the cost of duplicating some code) ?
</p>
TicketBurcin ErocalMon, 19 Sep 2011 16:21:45 GMT
https://trac.sagemath.org/ticket/9880#comment:54
https://trac.sagemath.org/ticket/9880#comment:54
<p>
I updated my patch-queue with some small but important fixes to the handling of these leading minuses. Please update and try that version. There updates were sitting on my hard disk for months, so I need some time to remember the details again.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:53" title="Comment 53">jpflori</a>:
</p>
<blockquote class="citation">
<p>
And isn't there a better way to check for leading minus than to emulate printing (even at the cost of duplicating some code) ?
</p>
</blockquote>
<p>
Since the coefficient can be any Sage type, I don't think there is any way to get that information without printing. IIRC, GiNaC handles this by defining the csgn() function appropriately, but they don't support as many types and printing styles as we do.
</p>
TicketJean-Pierre FloriMon, 19 Sep 2011 18:00:03 GMT
https://trac.sagemath.org/ticket/9880#comment:55
https://trac.sagemath.org/ticket/9880#comment:55
<p>
I think I used all the patch from your above link.
</p>
<p>
I'll have a closer look at it.
</p>
<p>
Did you also tried the examples I gave above?
</p>
TicketJean-Pierre FloriTue, 20 Sep 2011 09:47:17 GMT
https://trac.sagemath.org/ticket/9880#comment:56
https://trac.sagemath.org/ticket/9880#comment:56
<p>
My bad, I thought you spoke of your previous changes...
</p>
<p>
I just recloned your repo and all the above tests now pass.
</p>
TicketJean-Pierre FloriTue, 20 Sep 2011 11:17:18 GMT
https://trac.sagemath.org/ticket/9880#comment:57
https://trac.sagemath.org/ticket/9880#comment:57
<p>
Just a remark: some of your patches in the queue do not have an author set (I mean inside the Mercurial header).
</p>
TicketJean-Pierre FloriTue, 20 Sep 2011 15:41:18 GMT
https://trac.sagemath.org/ticket/9880#comment:58
https://trac.sagemath.org/ticket/9880#comment:58
<p>
The latest version of the patch giving access to the internal printing functions, gives the operator() instead of the compare(...) functions.
</p>
<p>
I'm not sure this was intended, even if it is completely ok for the randomized testing.
</p>
<p>
Anyway the line using it is cmp(..) == 1, so going back to the previous semantic is also ok.
</p>
<p>
Hence I've provided a modified patch to do it.
</p>
<p>
I'll also reversed the arguments in _cmp_c_impl and get rid of the minus sign.
</p>
<p>
I do not see any reason for this choice, but if there was, please revert my change.
</p>
TicketJean-Pierre FloriTue, 20 Sep 2011 15:42:07 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_comparison-p1.patch</em>
</li>
</ul>
TicketJean-Pierre FloriWed, 21 Sep 2011 09:05:41 GMT
https://trac.sagemath.org/ticket/9880#comment:59
https://trac.sagemath.org/ticket/9880#comment:59
<p>
There is a little glitch left in pynac which made SR(1) > SR(2).
</p>
TicketJean-Pierre FloriWed, 21 Sep 2011 09:07:07 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>numerics.patch</em>
</li>
</ul>
TicketJean-Pierre FloriWed, 21 Sep 2011 09:36:06 GMT
https://trac.sagemath.org/ticket/9880#comment:60
https://trac.sagemath.org/ticket/9880#comment:60
<p>
The "variable" ordering somehow got reversed at some point.
</p>
<p>
The following patch fixes that.
</p>
TicketJean-Pierre FloriWed, 21 Sep 2011 09:37:04 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_variables_ordering.patch</em>
</li>
</ul>
<p>
Revert previous ordering for "variables" function
</p>
TicketJean-Pierre FloriWed, 21 Sep 2011 09:40:07 GMT
https://trac.sagemath.org/ticket/9880#comment:61
https://trac.sagemath.org/ticket/9880#comment:61
<p>
Apart from changes in the printing order, we now get the following error:
</p>
<pre class="wiki">sage: print ((a+b)*(a+c)).match((w0+w1)*(w0+w2))
Expected:
{$2: b, $0: a, $1: c}
Got:
None
</pre>
TicketJean-Pierre FloriSun, 06 Nov 2011 12:12:38 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:62
https://trac.sagemath.org/ticket/9880#comment:62
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=62">diff</a>)
</li>
</ul>
TicketJean-Pierre FloriSun, 06 Nov 2011 12:17:12 GMTsummary changed
https://trac.sagemath.org/ticket/9880#comment:63
https://trac.sagemath.org/ticket/9880#comment:63
<ul>
<li><strong>summary</strong>
changed from <em>Segfault in PyNaC 0.2.0.p4</em> to <em>Pynac comparison functions do not provide a SWO</em>
</li>
</ul>
TicketBurcin ErocalFri, 25 Nov 2011 16:48:21 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_doctests_symbolic.take2.patch</em>
</li>
</ul>
TicketBurcin ErocalFri, 25 Nov 2011 16:48:47 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_doctests-be.patch</em>
</li>
</ul>
TicketBurcin ErocalFri, 25 Nov 2011 17:02:19 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:64
https://trac.sagemath.org/ticket/9880#comment:64
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=64">diff</a>)
</li>
</ul>
<p>
I've just pushed updates to my pynac patch queue that include Jean-Pierre's <code>numerics.patch</code>, a preliminary fix for <code>match()</code> and some attempts to fix the random effects of normalization of minus signs on printing. I don't expect all of these to be committed, they are work in progress.
</p>
<p>
I also uploaded a slightly changed version of Volker's doctest fix patch <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9880/trac_9880-fix_doctests_symbolic.take2.patch" title="Attachment 'trac_9880-fix_doctests_symbolic.take2.patch' in Ticket #9880">trac_9880-fix_doctests_symbolic.take2.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9880/trac_9880-fix_doctests_symbolic.take2.patch" title="Download"></a>, and a new patch <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9880/trac_9880-fix_doctests-be.patch" title="Attachment 'trac_9880-fix_doctests-be.patch' in Ticket #9880">trac_9880-fix_doctests-be.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9880/trac_9880-fix_doctests-be.patch" title="Download"></a> fixing many more doctests which I believe to be correct.
</p>
<p>
Running the test suite on <code>sage/{symbolic,function}</code> directories after these patches shows more clearly that there is still some work to be done here. We get some random output from <code>.factor()</code> and <code>.numerator()</code> calls (<a class="closed ticket" href="https://trac.sagemath.org/ticket/12068" title="#12068: enhancement: Numerator for symbolic expression shouldn't use maxima (closed: fixed)">#12068</a>), and a timeout on <code>sage/calculus/calculus.py</code> since <code>minpoly</code> doesn't terminate.
</p>
<p>
Here is my current sage patch queue in case you decide to apply the new patches:
</p>
<pre class="wiki">trac_12068-numer_denom_ginac-fh.patch
trac_12068-denominator.patch
trac_12074-nth_root.patch
trac_9880_fix_import.patch
trac_9880_pynac_order.take2.patch
trac_9880_randomized_testing.patch
trac_9880-stable_operands.patch
trac_9880_pynac_infinities.patch
trac_9880-fix_comparison-p1.patch
trac_9880-fix_variables_ordering.patch
trac_9880-fix_doctests_symbolic.patch
trac_9880-fix_doctests-be.patch
</pre><p>
I'll write to pynac-devel with more details of the remaining problems.
</p>
TicketJean-Pierre FloriSat, 26 Nov 2011 23:40:16 GMTdescription changed; dependencies set
https://trac.sagemath.org/ticket/9880#comment:65
https://trac.sagemath.org/ticket/9880#comment:65
<ul>
<li><strong>dependencies</strong>
set to <em>12068</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=65">diff</a>)
</li>
</ul>
TicketJean-Pierre FloriSat, 26 Nov 2011 23:40:45 GMTdependencies changed
https://trac.sagemath.org/ticket/9880#comment:66
https://trac.sagemath.org/ticket/9880#comment:66
<ul>
<li><strong>dependencies</strong>
changed from <em>12068</em> to <em>#12068</em>
</li>
</ul>
TicketJean-Pierre FloriSun, 27 Nov 2011 00:20:37 GMT
https://trac.sagemath.org/ticket/9880#comment:67
https://trac.sagemath.org/ticket/9880#comment:67
<p>
Here is a small example producing the random minus sign problem for anyone wanting to investigate it apart from Burcin and I:
</p>
<pre class="wiki">sage -c "var('x y z'); print (-x+z)*(3*x-3*z)"
</pre>
TicketBurcin ErocalTue, 10 Jul 2012 09:28:10 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:68
https://trac.sagemath.org/ticket/9880#comment:68
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=68">diff</a>)
</li>
</ul>
<p>
This fixes <a class="closed ticket" href="https://trac.sagemath.org/ticket/9046" title="#9046: defect: bug in collect and/or term ordering in symbolics (closed: duplicate)">#9046</a> as well. I attached <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9880/trac_9880-doctest_for_9046.patch" title="Attachment 'trac_9880-doctest_for_9046.patch' in Ticket #9880">trac_9880-doctest_for_9046.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9880/trac_9880-doctest_for_9046.patch" title="Download"></a> to add the example there as a doctest.
</p>
<p>
We don't need <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9880/trac_9880_pynac_infinities.patch" title="Attachment 'trac_9880_pynac_infinities.patch' in Ticket #9880">trac_9880_pynac_infinities.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9880/trac_9880_pynac_infinities.patch" title="Download"></a> since that was merged in <a class="closed ticket" href="https://trac.sagemath.org/ticket/12950" title="#12950: defect: update to Pynac 0.2.4 (closed: fixed)">#12950</a>, so I removed it from the patches listed in the description.
</p>
<p>
Naturally the doctest patches, the last two in the list, have bitrotted. The rest seems to apply fine, though I only tried with 5.0.beta13.
</p>
TicketBurcin ErocalWed, 21 Nov 2012 21:12:05 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880_pynac_order.take2.rebased-5.5.rc0.patch</em>
</li>
</ul>
TicketBurcin ErocalWed, 21 Nov 2012 21:12:31 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-stable_operands.rebased-5.5.rc0.patch</em>
</li>
</ul>
TicketBurcin ErocalWed, 21 Nov 2012 21:12:50 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_comparison-p1.rebased-5.5.rc0.patch</em>
</li>
</ul>
TicketBurcin ErocalWed, 21 Nov 2012 21:13:33 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_doctests_symbolic.take2.rebased-5.5.rc0.patch</em>
</li>
</ul>
TicketBurcin ErocalWed, 21 Nov 2012 21:13:53 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_doctests-be.rebased-5.5.rc0.patch</em>
</li>
</ul>
TicketBurcin ErocalWed, 21 Nov 2012 21:26:02 GMTdescription changed; dependencies deleted
https://trac.sagemath.org/ticket/9880#comment:69
https://trac.sagemath.org/ticket/9880#comment:69
<ul>
<li><strong>dependencies</strong>
<em>#12068</em> deleted
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=69">diff</a>)
</li>
</ul>
<p>
I rebased the patches to Sage 5.5.rc0, dropping doctest fixes that resulted in nontrivial conflicts on the way.
</p>
<p>
Volker's <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9880/trac_9880_fix_import.patch" title="Attachment 'trac_9880_fix_import.patch' in Ticket #9880">trac_9880_fix_import.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9880/trac_9880_fix_import.patch" title="Download"></a> is now on <a class="closed ticket" href="https://trac.sagemath.org/ticket/13737" title="#13737: defect: fix cython warning in pynac.pyx (closed: fixed)">#13737</a>.
</p>
<p>
My Sage patch queue contains Pynac 0.2.6 associated patches from <a class="closed ticket" href="https://trac.sagemath.org/ticket/13262" title="#13262: defect: Update doctests after bug correction in pynac (closed: fixed)">#13262</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/13609" title="#13609: defect: symbolic arithmetic errors (closed: fixed)">#13609</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/13587" title="#13587: defect: automatic simplification can lose some infinities (closed: fixed)">#13587</a>. The patches on this ticket might apply cleanly without those, I haven't tried.
</p>
TicketBurcin ErocalWed, 21 Nov 2012 21:29:41 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-doctest_for_9046.patch</em>
</li>
</ul>
TicketBurcin ErocalTue, 07 May 2013 16:48:56 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-doctest_for_9046-rebased-5.9.patch</em>
</li>
</ul>
TicketBurcin ErocalTue, 07 May 2013 16:49:18 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_doctests_symbolic.take2.rebased-5.9.patch</em>
</li>
</ul>
TicketBurcin ErocalTue, 07 May 2013 16:49:36 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_doctests-be.rebased-5.9.patch</em>
</li>
</ul>
TicketBurcin ErocalTue, 07 May 2013 16:56:37 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:70
https://trac.sagemath.org/ticket/9880#comment:70
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=70">diff</a>)
</li>
</ul>
<p>
I rebased the patches to 5.9.
</p>
<p>
Here is an example where <code>minpoly()</code> does not terminate:
</p>
<pre class="wiki">sage: var('x')
x
sage: eqn = x^3 + sqrt(2)*x + 5 == 0
sage: a = solve(eqn, x)[0].rhs()
sage: a
-1/2*(1/18*sqrt(3)*sqrt(8*sqrt(2) + 675) - 5/2)^(1/3)*(I*sqrt(3) + 1) - 1/6*sqrt(2)*(I*sqrt(3) - 1)/(1/18*sqrt(3)*sqrt(8*sqrt(2) + 675) - 5/2)^(1/3)
sage: QQ[a]
</pre><p>
or
</p>
<pre class="wiki">sage: a.minpoly()
</pre>
TicketBurcin ErocalThu, 09 May 2013 16:14:16 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_doctests-be-rebased-5.9-on_13213.patch</em>
</li>
</ul>
TicketBurcin ErocalThu, 09 May 2013 16:15:10 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-more_doctests-be.patch</em>
</li>
</ul>
TicketBurcin ErocalThu, 09 May 2013 16:28:04 GMTdescription changed; reviewer, dependencies, author set
https://trac.sagemath.org/ticket/9880#comment:71
https://trac.sagemath.org/ticket/9880#comment:71
<ul>
<li><strong>reviewer</strong>
set to <em>Burcin Erocal</em>
</li>
<li><strong>dependencies</strong>
set to <em>#13213</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=71">diff</a>)
</li>
<li><strong>author</strong>
set to <em>Burcin Erocal, Jean-Pierre Flori, Volker Braun</em>
</li>
</ul>
<p>
I think this is finally good to be tested.
</p>
<p>
There is a new Pynac spkg (it's just for testing) and updated patches on this ticket. I'm working with vanilla 5.9. Note that this ticket now depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/13213" title="#13213: enhancement: Comparisons in quadratic number field (closed: fixed)">#13213</a> for the fixes to the quadratic number field ordering.
</p>
<p>
Please test and report results.
</p>
TicketJean-Pierre FloriFri, 10 May 2013 14:24:59 GMTreviewer changed
https://trac.sagemath.org/ticket/9880#comment:72
https://trac.sagemath.org/ticket/9880#comment:72
<ul>
<li><strong>reviewer</strong>
changed from <em>Burcin Erocal</em> to <em>Burcin Erocal, Jean-Pierre Flori</em>
</li>
</ul>
<p>
Looks fine and some hand feeded examples pass!
I guess what's left to do is:
</p>
<ul><li>cleanup Volker's patch (blank lines, <a class="ext-link" href="http://...trac"><span class="icon"></span>http://...trac</a>... -> :trac:, becasue->because, EXAMPLE[S]: -> EXAMPLE[S]::)
</li><li>merge related paches to ease readibility for reviewers (=me?),
</li><li>add some doctests using previously failing examples: from <a class="closed ticket" href="https://trac.sagemath.org/ticket/10833" title="#10833: defect: Unhandled SIGSEGV on large expand of iterated polynomial (closed: duplicate)">#10833</a>, from ticket description, from comment 15 (<a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/9880#comment:15"><span class="icon"></span>http://trac.sagemath.org/sage_trac/ticket/9880#comment:15</a>), maybe 52 <a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/9880#comment:52"><span class="icon"></span>http://trac.sagemath.org/sage_trac/ticket/9880#comment:52</a> and 67 <a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/9880#comment:67"><span class="icon"></span>http://trac.sagemath.org/sage_trac/ticket/9880#comment:67</a>
</li><li>craft a proper spkg
</li><li>make sure it cleanly applies on 5.10.beta2 and passes all doctests
</li><li>positive review!
</li></ul>
TicketBurcin ErocalFri, 10 May 2013 17:03:00 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880_pynac_order-sage_5_10_beta2.patch</em>
</li>
</ul>
TicketBurcin ErocalFri, 10 May 2013 17:03:19 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880_randomized_testing-sage_5_10_beta2.patch</em>
</li>
</ul>
TicketBurcin ErocalFri, 10 May 2013 17:03:36 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-doctest_for_9046-sage_5_10_beta2.patch</em>
</li>
</ul>
TicketBurcin ErocalFri, 10 May 2013 17:03:56 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_doctests-sage_5_10_beta2.patch</em>
</li>
</ul>
TicketBurcin ErocalFri, 10 May 2013 17:04:15 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-add_doctests-sage_5_10_beta2.patch</em>
</li>
</ul>
TicketBurcin ErocalFri, 10 May 2013 17:11:59 GMTstatus, description changed
https://trac.sagemath.org/ticket/9880#comment:73
https://trac.sagemath.org/ticket/9880#comment:73
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=73">diff</a>)
</li>
</ul>
<p>
I uploaded new patches that do almost everything suggested in <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:72" title="Comment 72">comment:72</a>. Remaining items are:
</p>
<ul><li>make a proper spkg
</li><li>check all doctests
</li></ul><p>
I will make a new Pynac release and upload the tarball to the web site soon. Running doctests on the whole tree now.
</p>
<p>
I'm setting this to <code>needs_review</code>. Please test and report any problems.
</p>
TicketVolker BraunFri, 10 May 2013 18:10:28 GMT
https://trac.sagemath.org/ticket/9880#comment:74
https://trac.sagemath.org/ticket/9880#comment:74
<p>
I get a bunch of doctest failures because of changed term orders, stuff like
</p>
<pre class="wiki">File "devel/sage/sage/plot/plot3d/plot3d.py", line 418, in sage.plot.plot3d.plot3d.Spherical
Failed example:
T.transform(radius=r, azimuth=theta, inclination=phi)
Expected:
(r*sin(phi)*cos(theta), r*sin(phi)*sin(theta), r*cos(phi))
Got:
(r*cos(theta)*sin(phi), r*sin(phi)*sin(theta), r*cos(phi))
</pre>
TicketJean-Pierre FloriMon, 13 May 2013 10:51:02 GMT
https://trac.sagemath.org/ticket/9880#comment:75
https://trac.sagemath.org/ticket/9880#comment:75
<p>
Yup, Burcin only corrected the doctests in the symbolic or something like that.
</p>
<p>
Burcin, are you working on this or do you prefer that I take care of correcting the doctests?
I had a look and it really looks like ordering change only, so is harmless and trivial to change.
</p>
<p>
By the way, I'm ok with all the other patches, so this should get in once we fix the doctests!
</p>
TicketBurcin ErocalMon, 13 May 2013 21:59:15 GMT
https://trac.sagemath.org/ticket/9880#comment:76
https://trac.sagemath.org/ticket/9880#comment:76
<p>
I only checked the <code>sage/symbolics</code> directory and a few other files, then switched to <code>needs_review</code> to see if anyone saw major problems before changing all the other doctests. Then travel and real life came in. I will only be able to look at this again Thursday afternoon. If you have time to take care of the remaining doctests, please go ahead.
</p>
<p>
BTW, we still need <a class="closed ticket" href="https://trac.sagemath.org/ticket/13213" title="#13213: enhancement: Comparisons in quadratic number field (closed: fixed)">#13213</a> reviewed before this can go in.
</p>
TicketJean-Pierre FloriThu, 16 May 2013 20:33:35 GMT
https://trac.sagemath.org/ticket/9880#comment:77
https://trac.sagemath.org/ticket/9880#comment:77
<p>
I've had a quick look at the failing doctests and think there is no real problem hidden there.
</p>
<p>
The only thing which may be a little distrubing is that now we print
</p>
<pre class="wiki">(2*x+1)*x^2
</pre><p>
rather than
</p>
<pre class="wiki">x^2*(2*x+1)
</pre><p>
This should be easily fixed in pynac hopefully, but even if not I don't think it should be a blocker.
</p>
<p>
The other disturbing thing is than fractions get automatically modified when they used not to be:
</p>
<pre class="wiki">File "devel/sage/doc/ru/tutorial/introduction.rst", line 49, in doc.ru.tutorial.introduction
Failed example:
k = 1/(sqrt(3)*I + 3/4 + sqrt(73)*5/9); k
Expected:
1/(I*sqrt(3) + 5/9*sqrt(73) + 3/4)
Got:
36/(20*sqrt(73) + 36*I*sqrt(3) + 27)
</pre><p>
Apart from that we also get a lot of nicer things now (in addition to bug fixes) than before.
</p>
TicketPaul ZimmermannTue, 21 May 2013 18:52:37 GMTcc changed
https://trac.sagemath.org/ticket/9880#comment:78
https://trac.sagemath.org/ticket/9880#comment:78
<ul>
<li><strong>cc</strong>
<em>Paul Zimmermann</em> added
</li>
</ul>
TicketPaul ZimmermannWed, 22 May 2013 08:45:55 GMT
https://trac.sagemath.org/ticket/9880#comment:79
https://trac.sagemath.org/ticket/9880#comment:79
<p>
I cannot review this patch for the doctests of our book in french, since the dependency patch
<a class="closed ticket" href="https://trac.sagemath.org/ticket/13213" title="#13213: enhancement: Comparisons in quadratic number field (closed: fixed)">#13213</a> does not apply to Sage 5.9 (which is the version we use in the book):
</p>
<pre class="wiki">applying /tmp/trac_13213-quadratic_field_comparison.patch
patching file sage/schemes/elliptic_curves/ell_number_field.py
Hunk #1 FAILED at 1347
1 out of 3 hunks FAILED -- saving rejects to file sage/schemes/elliptic_curves/ell_number_field.py.rej
abort: patch failed to apply
</pre><p>
Paul
</p>
TicketBurcin ErocalThu, 23 May 2013 11:11:05 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_doctests-sage_5_10_beta2.take2.patch</em>
</li>
</ul>
TicketBurcin ErocalThu, 23 May 2013 11:13:39 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:80
https://trac.sagemath.org/ticket/9880#comment:80
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=80">diff</a>)
</li>
</ul>
<p>
I uploaded a new patch <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9880/" title="Attachments of Ticket #9880">trac_9880-fix_doctests-sage_5_10_beta2.take2.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9880/" title="Download"></a> to fix all doctest in the library (as opposed to the few symbolics related directories I was working with).
</p>
<p>
Now all that remains is to make a proper spkg, and someone to look over the doctest changes to see if there is anything fishy.
</p>
TicketJean-Pierre FloriThu, 23 May 2013 12:46:25 GMT
https://trac.sagemath.org/ticket/9880#comment:81
https://trac.sagemath.org/ticket/9880#comment:81
<p>
Thanks Burcin!
I was finally going to do it today, but you were faster than me.
At least I can give it a try on beta4 and check there is nothing else to fix (all other patches apply cleanly.
</p>
<p>
I'd really like other people (let's say at least one) to have a look at the new order before you finalize 3.0 (or if it's already finalized/tagged in your repo make a 3.1 or 3.0.1) in case we want to make minimal changes.
E.g. I don't reallylike the following, but do not really mind as well:
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:77" title="Comment 77">jpflori</a>:
</p>
<blockquote class="citation">
<p>
The only thing which may be a little distrubing is that now we print
</p>
<pre class="wiki">(2*x+1)*x^2
</pre><p>
rather than
</p>
<pre class="wiki">x^2*(2*x+1)
</pre><p>
This should be easily fixed in pynac hopefully, but even if not I don't think it should be a blocker.
</p>
</blockquote>
<p>
But it would be really better to make these changes, if we plan to make them, before merging the ticket, so that people preparing books (I'm thinking of the french book which is in the process of being published) can expect their examples to pass for a quite long amount of time (or at least not to fail in the next version of Sage).
</p>
TicketBurcin ErocalSun, 26 May 2013 10:06:12 GMT
https://trac.sagemath.org/ticket/9880#comment:82
https://trac.sagemath.org/ticket/9880#comment:82
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:81" title="Comment 81">jpflori</a>:
</p>
<blockquote class="citation">
<p>
Thanks Burcin!
I was finally going to do it today, but you were faster than me.
At least I can give it a try on beta4 and check there is nothing else to fix (all other patches apply cleanly.
</p>
<p>
I'd really like other people (let's say at least one) to have a look at the new order before you finalize 3.0 (or if it's already finalized/tagged in your repo make a 3.1 or 3.0.1) in case we want to make minimal changes.
</p>
</blockquote>
<p>
I agree. This is the reason I set this ticket to needs review even though it was failing tests all over the library. :)
</p>
<p>
I tagged the 0.3.0 release already, but I can easily make a 0.3.1 if necessary.
</p>
<blockquote class="citation">
<p>
E.g. I don't reallylike the following, but do not really mind as well:
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:77" title="Comment 77">jpflori</a>:
</p>
<blockquote class="citation">
<p>
The only thing which may be a little distrubing is that now we print
</p>
<pre class="wiki">(2*x+1)*x^2
</pre><p>
rather than
</p>
<pre class="wiki">x^2*(2*x+1)
</pre><p>
This should be easily fixed in pynac hopefully, but even if not I don't think it should be a blocker.
</p>
</blockquote>
<p>
But it would be really better to make these changes, if we plan to make them, before merging the ticket, so that people preparing books (I'm thinking of the french book which is in the process of being published) can expect their examples to pass for a quite long amount of time (or at least not to fail in the next version of Sage).
</p>
</blockquote>
<p>
Feel free to submit patches to pynac with necessary changes. I don't have time to work on this any more right now, but I can spare some minutes to make a new release with simple patches.
</p>
<p>
That said, I'd really like this ticket to go in as soon as possible. It really is not fun to maintain the patch with the doctest fixes.
</p>
TicketJean-Pierre FloriMon, 27 May 2013 12:31:10 GMT
https://trac.sagemath.org/ticket/9880#comment:83
https://trac.sagemath.org/ticket/9880#comment:83
<p>
Burcin, could you reup a proper spkg? i.e. with just an additional line in SPKG.txt and an hg commit message?
</p>
TicketBurcin ErocalMon, 27 May 2013 13:23:06 GMT
https://trac.sagemath.org/ticket/9880#comment:84
https://trac.sagemath.org/ticket/9880#comment:84
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:83" title="Comment 83">jpflori</a>:
</p>
<blockquote class="citation">
<p>
Burcin, could you reup a proper spkg? i.e. with just an additional line in SPKG.txt and an hg commit message?
</p>
</blockquote>
<p>
I updated the spkg at the URL given in the description with a new one.
</p>
<p>
Many thanks for reviewing this!
</p>
TicketJean-Pierre FloriMon, 27 May 2013 13:25:00 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_doctests-sage_5_10_beta5.patch</em>
</li>
</ul>
<p>
Updated patch to apply to 5.10.beta5 without fuzz
</p>
TicketJean-Pierre FloriMon, 27 May 2013 13:25:25 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-review.patch</em>
</li>
</ul>
<p>
Reviewer patch, fix two doctests.
</p>
TicketJean-Pierre FloriMon, 27 May 2013 13:26:14 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:85
https://trac.sagemath.org/ticket/9880#comment:85
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=85">diff</a>)
</li>
</ul>
TicketJean-Pierre FloriMon, 27 May 2013 13:31:29 GMT
https://trac.sagemath.org/ticket/9880#comment:86
https://trac.sagemath.org/ticket/9880#comment:86
<p>
A very little rant: could you mention the ticket number in the hg commit message as well, I know it's already in SPKG.txt but for lazy people as me it avoids to have to run two commands (if unlucky) before finding out the trac ticket number?
And I think it's not necessary to tag the spkg new version, Jeroen scripts will do it automatically.
I don't say it's a good thing, but it's the way it is now.
Nevertheless, tagging the spkg yourself shouldn't be an issue IIRC.
</p>
TicketBurcin ErocalMon, 27 May 2013 17:47:36 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:87
https://trac.sagemath.org/ticket/9880#comment:87
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=87">diff</a>)
</li>
</ul>
<p>
It's been quite a while since the last time I made an <code>spkg</code>. New version is up at the same URL with a ticket number in the commit message, and no tags. :)
</p>
<p>
That reminds me... I need to update the pynac web site to mention the release.
</p>
TicketJean-Pierre FloriMon, 27 May 2013 19:04:15 GMT
https://trac.sagemath.org/ticket/9880#comment:88
https://trac.sagemath.org/ticket/9880#comment:88
<p>
It seems the tag was added to the last commit and you then removed it.
Anyway, I'll just repackage it properly.
</p>
TicketJean-Pierre FloriMon, 27 May 2013 19:39:16 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>pynac-0.3.0.diff</em>
</li>
</ul>
<p>
Spkg diff, for review only.
</p>
TicketJean-Pierre FloriMon, 27 May 2013 19:40:30 GMTstatus, keywords, description changed
https://trac.sagemath.org/ticket/9880#comment:89
https://trac.sagemath.org/ticket/9880#comment:89
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>keywords</strong>
<em>spkg</em> added
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=89">diff</a>)
</li>
</ul>
TicketJeroen DemeyerMon, 27 May 2013 20:16:41 GMTmilestone changed
https://trac.sagemath.org/ticket/9880#comment:90
https://trac.sagemath.org/ticket/9880#comment:90
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.10</em> to <em>sage-5.11</em>
</li>
</ul>
TicketJeroen DemeyerTue, 28 May 2013 20:29:24 GMTstatus, dependencies changed
https://trac.sagemath.org/ticket/9880#comment:91
https://trac.sagemath.org/ticket/9880#comment:91
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
<li><strong>dependencies</strong>
changed from <em>#13213</em> to <em>#13213, #14550</em>
</li>
</ul>
<p>
Please rebase to <a class="closed ticket" href="https://trac.sagemath.org/ticket/14550" title="#14550: enhancement: German tutorial for school teachers (mainly aimed at Gymnasium level) (closed: fixed)">#14550</a>.
</p>
TicketBurcin ErocalWed, 29 May 2013 08:07:56 GMTstatus, description changed
https://trac.sagemath.org/ticket/9880#comment:92
https://trac.sagemath.org/ticket/9880#comment:92
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=92">diff</a>)
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:91" title="Comment 91">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Please rebase to <a class="closed ticket" href="https://trac.sagemath.org/ticket/14550" title="#14550: enhancement: German tutorial for school teachers (mainly aimed at Gymnasium level) (closed: fixed)">#14550</a>.
</p>
</blockquote>
<p>
Done.
</p>
TicketJean-Pierre FloriWed, 29 May 2013 09:29:31 GMTdescription changed
https://trac.sagemath.org/ticket/9880#comment:93
https://trac.sagemath.org/ticket/9880#comment:93
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=93">diff</a>)
</li>
</ul>
TicketJeroen DemeyerWed, 29 May 2013 12:33:25 GMTstatus, dependencies changed
https://trac.sagemath.org/ticket/9880#comment:94
https://trac.sagemath.org/ticket/9880#comment:94
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
<li><strong>dependencies</strong>
changed from <em>#13213, #14550</em> to <em>#13213, #9890, #14550</em>
</li>
</ul>
<p>
Please rebase to <a class="closed ticket" href="https://trac.sagemath.org/ticket/9890" title="#9890: defect: A proper random_element() method for PerfectMatchings (closed: fixed)">#9890</a> (the latest patch applies with fuzz 2).
</p>
TicketBurcin ErocalWed, 29 May 2013 12:55:52 GMTattachment set
https://trac.sagemath.org/ticket/9880
https://trac.sagemath.org/ticket/9880
<ul>
<li><strong>attachment</strong>
set to <em>trac_9880-fix_doctests-sage_5_10_beta2.take3.patch</em>
</li>
</ul>
TicketBurcin ErocalWed, 29 May 2013 12:56:44 GMTstatus, description changed
https://trac.sagemath.org/ticket/9880#comment:95
https://trac.sagemath.org/ticket/9880#comment:95
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9880?action=diff&version=95">diff</a>)
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9880#comment:94" title="Comment 94">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Please rebase to <a class="closed ticket" href="https://trac.sagemath.org/ticket/9890" title="#9890: defect: A proper random_element() method for PerfectMatchings (closed: fixed)">#9890</a> (the latest patch applies with fuzz 2).
</p>
</blockquote>
<p>
Done.
</p>
TicketJeroen DemeyerThu, 06 Jun 2013 12:35:30 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/9880#comment:96
https://trac.sagemath.org/ticket/9880#comment:96
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.11.beta0</em>
</li>
</ul>
Ticket