Sage: Ticket #11316: Weighted degree term orders added
https://trac.sagemath.org/ticket/11316
<p>
Weighted degree term orders (wp,Wp,ws,Ws as in Singular) added to <a class="missing wiki">TermOrder?</a>.
</p>
<p>
New term orders as well as matrix term orders can be used in block term orders.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/11316
Trac 1.1.6kleeMon, 09 May 2011 09:48:49 GMTstatus changed; author set
https://trac.sagemath.org/ticket/11316#comment:1
https://trac.sagemath.org/ticket/11316#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>author</strong>
set to <em>Kwankyu Lee</em>
</li>
</ul>
<p>
I think that using a method is a preferred way to get a property of an object rather than using an attribute, in Sage. So the attribute "blocks" of <a class="missing wiki">TermOrder?</a> is now a method. Use
</p>
<p>
t.blocks()
</p>
<p>
instead of
</p>
<p>
t.blocks
</p>
<p>
Perhaps this change will require a deprecation warning. I am not sure how to do this for attributes.
</p>
TicketkleeTue, 10 May 2011 05:24:59 GMTstatus changed
https://trac.sagemath.org/ticket/11316#comment:2
https://trac.sagemath.org/ticket/11316#comment:2
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
TicketkleeTue, 10 May 2011 08:03:24 GMTstatus changed
https://trac.sagemath.org/ticket/11316#comment:3
https://trac.sagemath.org/ticket/11316#comment:3
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Regarding to converting "blocks" to "blocks()", Jason remarks:
</p>
<p>
To my understanding, we prefer methods to properties because:
</p>
<ol><li>Methods have docstrings and can be introspected (using question mark)
</li></ol><p>
to obtain the docs.
</p>
<ol start="2"><li>We want to be consistent, as it can be confusing for a user if some
</li></ol><p>
things require parentheses (methods) while others don't (properties).
</p>
<p>
I really wish we could use properties more, as it is more pythonic and
looks cleaner, but those are two good points above...
</p>
TicketmalbWed, 18 May 2011 12:33:41 GMTstatus changed
https://trac.sagemath.org/ticket/11316#comment:4
https://trac.sagemath.org/ticket/11316#comment:4
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<ul><li>the patch does not apply cleanly against 4.7.rc2, but the failure is minor: only an update of a docstring in <code>pbori.pyx</code> fails.
</li><li>Doctests do not pass for 4.7.rc2:
<ul><li><code>pbori.pyx</code> I didn't fix the patch to apply cleanly, hence was to be expected that this one fails.
</li><li><code>sage_object.pyx</code> the patch seems to break pickling
</li></ul></li></ul><p>
So, the only real issue seems to be pickling.
</p>
TicketkleeFri, 20 May 2011 02:49:22 GMTstatus changed
https://trac.sagemath.org/ticket/11316#comment:5
https://trac.sagemath.org/ticket/11316#comment:5
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I rebased the patch on 4.7.rc2. pbori.pyx is no longer a problem.
</p>
<p>
I am not familiar with pickling. As I understand it, the patch modified the data structure of <a class="missing wiki">TermOrder?</a> objects in such an incompatible way that the old pickled <a class="missing wiki">TermOrder?</a> objects stored in the "pickle jar" of Sage is no longer properly unpickled. So it is not a bug of the patch but an incompatible change in the data structure that cause the unpickling failures. Please confirm this.
</p>
<p>
I don't know what to do with this. A quick way to fix this is just to update the "pickle jar" of Sage, and, perhaps, this is up to the release manager, and perhaps a consensus in sage-devel is required. My reference is
</p>
<p>
<a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/10768"><span class="icon"></span>http://trac.sagemath.org/sage_trac/ticket/10768</a>
</p>
<p>
I change the status of this ticket to "needs review" to get attention. Is "needs info" the right status?
</p>
TicketmalbFri, 20 May 2011 07:26:40 GMTstatus changed
https://trac.sagemath.org/ticket/11316#comment:6
https://trac.sagemath.org/ticket/11316#comment:6
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11316#comment:5" title="Comment 5">klee</a>:
</p>
<blockquote class="citation">
<p>
I am not familiar with pickling. As I understand it, the patch modified the data
structure of <a class="missing wiki">TermOrder?</a> objects in such an incompatible way that the old pickled
<a class="missing wiki">TermOrder?</a> objects stored in the "pickle jar" of Sage is no longer properly
unpickled. So it is not a bug of the patch but an incompatible change in the data
structure that cause the unpickling failures. Please confirm this.
</p>
</blockquote>
<p>
Your technical description seems right. However, in Sage's convention it is considered a bug if the pickle jar breaks.
</p>
<blockquote class="citation">
<p>
I don't know what to do with this. A quick way to fix this is just to update the
"pickle jar" of Sage
</p>
</blockquote>
<p>
This is not what is supposed to happen. The point of the pickle jar is to ensure old objects can be unpickled.
</p>
<blockquote class="citation">
<p>
, and, perhaps, this is up to the release manager, and perhaps a consensus in sage-
</p>
<blockquote>
<p>
devel is required. My reference is
</p>
</blockquote>
<p>
</p>
<blockquote>
<p>
<a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/10768"><span class="icon"></span>http://trac.sagemath.org/sage_trac/ticket/10768</a>
</p>
</blockquote>
<p>
</p>
<blockquote>
<p>
I change the status of this ticket to "needs review" to get attention. Is "needs info" the right status?
</p>
</blockquote>
</blockquote>
<p>
<strong>needs info</strong> or <strong>needs work</strong> are both fine as far as I understand it.
</p>
<p>
You can start debugging it by calling
</p>
<pre class="wiki">sage.structure.sage_object.unpickle_all("YOUR_SAGE_ROOT/data/extcode/pickle_jar/pickle_jar.tar.bz2")
</pre><p>
I'll also try to take a look soon-ish.
</p>
TicketkleeFri, 20 May 2011 08:06:53 GMTattachment set
https://trac.sagemath.org/ticket/11316
https://trac.sagemath.org/ticket/11316
<ul>
<li><strong>attachment</strong>
set to <em>trac_11316.patch</em>
</li>
</ul>
TicketkleeFri, 20 May 2011 08:12:10 GMT
https://trac.sagemath.org/ticket/11316#comment:7
https://trac.sagemath.org/ticket/11316#comment:7
<p>
There was a bug in the matrix order of Sage, which did not allow negative integers in the matrix. This is fixed in the current patch.
</p>
TicketmalbFri, 20 May 2011 14:02:52 GMT
https://trac.sagemath.org/ticket/11316#comment:8
https://trac.sagemath.org/ticket/11316#comment:8
<p>
btw. I read the patch and it looks good. The only thing I don't like are the double underscore attributes (most of which you inherited from other people like me). But we should add more of those, so I'd suggest at least <code>__weights</code> to become <code>_weights</code>. If you're up for changing the remaining ones as well, that'd be great (but no requirement of course) ... it'd also break pickling more I guess.
</p>
TicketburcinSat, 21 May 2011 09:37:32 GMTcc set
https://trac.sagemath.org/ticket/11316#comment:9
https://trac.sagemath.org/ticket/11316#comment:9
<ul>
<li><strong>cc</strong>
<em>burcin</em> added
</li>
</ul>
TicketmalbSat, 21 May 2011 10:51:17 GMT
https://trac.sagemath.org/ticket/11316#comment:10
https://trac.sagemath.org/ticket/11316#comment:10
<p>
Perhaps, this is a good way of dealing with this:
</p>
<div class="wiki-code"><div class="code"><pre><span class="k">def</span> <span class="nf">__getattr__</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span><span class="s">'name'</span><span class="p">):</span>
<span class="k">if</span> name <span class="o">==</span> <span class="s">"_weights"</span><span class="p">:</span> <span class="c"># I don't think it works for double underscore attributes?</span>
<span class="bp">self</span><span class="o">.</span>_weights <span class="o">=</span> <span class="bp">None</span>
<span class="k">return</span> <span class="bp">self</span><span class="o">.</span>_weights
<span class="k">else</span><span class="p">:</span>
<span class="k">raise</span> <span class="ne">AttributeError</span><span class="p">(</span><span class="s">"'TermOrder' has no attribute '</span><span class="si">%s</span><span class="s">'"</span><span class="o">%</span>name<span class="p">)</span>
</pre></div></div>
TicketkleeSun, 29 May 2011 05:06:43 GMTattachment set
https://trac.sagemath.org/ticket/11316
https://trac.sagemath.org/ticket/11316
<ul>
<li><strong>attachment</strong>
set to <em>trac_11316.2.patch</em>
</li>
</ul>
<p>
fixed the unpickling failures
</p>
TicketkleeSun, 29 May 2011 05:13:45 GMTstatus changed
https://trac.sagemath.org/ticket/11316#comment:11
https://trac.sagemath.org/ticket/11316#comment:11
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
I kept double underscores in the new patch. I don't know why Martin do not like double underscore attributes. Do you think single underscore attributes are generally preferable over double ones? Or is it just for fixing unpickling problem.
</p>
TicketkleeSun, 29 May 2011 05:35:54 GMT
https://trac.sagemath.org/ticket/11316#comment:12
https://trac.sagemath.org/ticket/11316#comment:12
<p>
For record,
</p>
<p>
<a class="ext-link" href="https://groups.google.com/forum/#!topic/sage-devel/z2r5ag-j1WY"><span class="icon"></span>https://groups.google.com/forum/#!topic/sage-devel/z2r5ag-j1WY</a>
</p>
TicketmalbSun, 29 May 2011 07:52:02 GMT
https://trac.sagemath.org/ticket/11316#comment:13
https://trac.sagemath.org/ticket/11316#comment:13
<p>
Re double underscores: they make inheriting from an object harder, since a subclass cannot easily access "_weights". We seem to have a bias in Sage towards single underscores.
</p>
TicketSimonKingSun, 29 May 2011 08:45:46 GMT
https://trac.sagemath.org/ticket/11316#comment:14
https://trac.sagemath.org/ticket/11316#comment:14
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11316#comment:11" title="Comment 11">klee</a>:
</p>
<blockquote class="citation">
<p>
I kept double underscores in the new patch. I don't know why Martin do not like double underscore attributes. Do you think single underscore attributes are generally preferable over double ones? Or is it just for fixing unpickling problem.
</p>
</blockquote>
<p>
As Martin said, double underscore attributes can be a problem in sub-classes. To be precise: We talk about an attribute whose name starts with two underscores but ends with less than two underscores. It is a Python convention that those attributes are private. And in order to emphasize the privacy, Python applies so-called name mangling (easy to google): The attribute name is mangled with the name of the class <em>for which it was originally defined</em>.
</p>
<p>
By consequence, there is a problem for subclasses:
</p>
<pre class="wiki">sage: class A:
....: def __init__(self,n):
....: self.__n = n
....:
sage: class B(A):
....: def __init__(self,n):
....: A.__init__(self,n^2)
....:
sage: b = B(5)
sage: b.__n
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
/home/king/<ipython console> in <module>()
AttributeError: B instance has no attribute '__n'
</pre><p>
So, surprise: <code>__n</code> seems gone. But it is there, under a different name:
</p>
<pre class="wiki">sage: b._A__n
25
</pre><p>
It is obvious that, under these circumstances, single underscore names are easier to deal with.
</p>
<p>
Remark: No mangling occurs if the name <em>ends</em> with two underscores. That's why all the magical methods <code>__repr__</code>, <code>__add__</code> etc. can be easily inherited.
</p>
TicketSimonKingSun, 29 May 2011 08:57:45 GMT
https://trac.sagemath.org/ticket/11316#comment:15
https://trac.sagemath.org/ticket/11316#comment:15
<p>
Concerning unpickling of old pickles, I think the following holds in this case:
</p>
<p>
The pickling of a <code>TermOrder</code> is actually quite simple, standard Python.
It is an object with a <code>__dict__</code>, and a new un-initialised instance can be created with <code>__new__</code>.
</p>
<p>
<strong>Example</strong>
</p>
<p>
First, we obtain an order:
</p>
<pre class="wiki">sage: P.<x,y>=QQ[]
sage: T = P.term_order()
</pre><p>
Now, if I am not mistaken, <code>S=loads(dumps(T))</code> essentially boils down to
the following.
First, we create a new instance of the class:
</p>
<pre class="wiki">sage: S = T.__new__(T.__class__)
</pre><p>
And then, we provide the new instance with a copy of the old
instance's <code>__dict__</code>:
</p>
<pre class="wiki">sage: S.__dict__ = copy(T.__dict__)
</pre><p>
As a result, we have a copy of T:
</p>
<pre class="wiki"> sage: S
Degree reverse lexicographic term order
sage: S == T
True
sage: S is T
False
</pre><p>
If I understand correctly, the problem here is that your patch
introduces a new attribute <code>_weights</code>, that has a default value for
trivial degree weights. So, if <code>__dict__</code> was taken from an old pickle,
then it lacks the key <code>'_weights'</code>. Hence, requesting <code>S._weights</code> would
fail.
</p>
<p>
Two solutions were suggested:
</p>
<ol><li>Introduce a <code>__getattr__</code> method, that returns the default weights and puts them into <code>S.__dict__['_weights']</code>. That ensures that <code>S.__getattr__('_weights')</code> is called at most once: <code>__getattr__</code> is only called if an attribute can not be found in the <code>__dict__</code> and not as a class attribute.
</li></ol><ol start="2"><li>Introduce the default <code>_weights</code> as a class attribute. If a term order has non-default weights then they are put into <code>__dict__</code> (this is what happens if you do <code>T._weights = [1,2,3]</code>). But attributes found in <code>__dict__</code> have precedence over class attributes. Hence, the class attribute will only be considered if <code>_weights</code> is not in <code>__dict__</code>.
</li></ol><p>
It seems to me that in both cases unpickling of old pickles should just work. One should test, though, whether one of the solutions has a speed penalty. I reckon that introducing <code>__getattr__</code> may slow things down. How often are attributes of a term order called when you do polynomial arithmetic?
</p>
TicketSimonKingSun, 29 May 2011 17:06:15 GMT
https://trac.sagemath.org/ticket/11316#comment:16
https://trac.sagemath.org/ticket/11316#comment:16
<p>
I was reading part of the second patch. So, this is only part of a review.
</p>
<p>
Concerning double versus single underscore: I would not insist on a single underscore. After all, Python has the concept of private attributes for a reason, and certainly the degree weights are private. However, if I write code then I usually avoid double underscores, because I don't like the name mangling. I guess it's a matter of taste.
</p>
<p>
Certainly, one way (not necessarily the easiest way) to solve the unpickling problem is to write a <code>__setstate__</code> method. That is what you do in the second patch. However, it seems awkward to create a whole new termorder inside <code>__setstate__</code> (line 523 of the second patch) just in order to get a value to update a dictionary with. Wouldn't it be easier to compute the missing item (the default value of <code>__weights</code>) directly?
</p>
<p>
In addition, it seems to me that one does not need to introduce a <code>__setstate__</code> method in order to solve the unpickling problems. Introducing a class attribute <code>_weights</code> (resp. <code>__weights</code>, but I don't know if there is a pitfall related with name mangling of class attributes) providing a default value should be enough to solve the problem. If one follows that approach, probably the default value would be None. Then, the methods using <code>__weights</code> should be modified so that the case <code>__weights==None</code> is correctly dealt with.
</p>
TicketkleeMon, 30 May 2011 00:54:39 GMT
https://trac.sagemath.org/ticket/11316#comment:17
https://trac.sagemath.org/ticket/11316#comment:17
<p>
To Simon,
</p>
<p>
I should say, I also don't like double underscored attributes. I just inherited them from previous authors of the class.
</p>
<p>
The unpickling problem is more complicated than what you think. There is a change in not only in the data structure but also in the logic. I made this change in order to deal with block term orders of whatever constituent term orders.
</p>
<p>
Therefore the solution you suggest is not suitable to solve this problem. Also in my point of view, even if it's possible, it would introduce lots of code into the main body of the <a class="missing wiki">TermOrder?</a> class, just to solve the unpickling problem of old objects!
</p>
<p>
I believe using <span class="underline">setstate</span> method is the most simple and natural way to solve the problem. Actually, this is the way the Python documentation, which I quoted in the sage-dev thread, recommends to solve such problems. The documentation also suggest to put the version information into the pickled objects, which I think is a nice way to build the infrastructure that I suggested in the sage-dev thread. Let me expound on this more in the following.
</p>
<p>
Let's assume pickled objects have "_version" attribute with them. This attribute may be added only just before pickling. Then a developer, who made a big change in the logic and data of the class of the objects, add a translating method for the objects pickled before this change to the class, perhaps with a name like "_translate_for_sage_4_7" or "_upgrade_for_sage_4_7". The translating method is automatically invoked by <span class="underline">setstate</span> method in the <a class="missing wiki">SageObject?</a> class when unpickling objects before Sage 4.7, perhaps with issuing a warning message to the user. Another developer may add another translating method like "_translate_for_sage_4_8". Then unpickling procedure invoke the two traslating method sequentially so that old objects is translated first for Sage 4.7 then for Sage 4.8. Of course, this is only sketchy and to be elaborated much.
</p>
<p>
I guess without such an infrastructure, developers will get discouraged to change internals of classes to enhance Sage, because of the inevitable unpickling problem.
</p>
<p>
By the way, it is easy to change the double underscored attributes into single underscored ones with <span class="underline">setstate</span> method.
</p>
TicketkleeMon, 30 May 2011 06:51:22 GMTattachment set
https://trac.sagemath.org/ticket/11316
https://trac.sagemath.org/ticket/11316
<ul>
<li><strong>attachment</strong>
set to <em>trac_11316.3.patch</em>
</li>
</ul>
<p>
rebased to Sage 4.7; replaced double underscored attributes to single underscored ones
</p>
TicketSimonKingMon, 30 May 2011 07:18:03 GMT
https://trac.sagemath.org/ticket/11316#comment:18
https://trac.sagemath.org/ticket/11316#comment:18
<p>
Before I submit my post: I am very tired, and that may imply that my text became more harsh than I intended it to be. I am sorry if this is the case.
</p>
<p>
To sum it up:
</p>
<ol><li>Concerning data structure: You merely add one attribute with a default value. That's just a trivial change of data structure. Certainly it can not seriously be named a change in logic.
</li></ol><ol start="2"><li>Breaking the pickle jar for the sake of the trivial addition of an attribute is a no-go.
</li></ol><ol start="3"><li>As much as I see, absolutely <em>any</em> unpickling problem can be solved without to change the main code body.
</li></ol><ol start="4"><li>Here, the unpickling problem could be solved by replacing your <code>__setstate__</code> method with <em>a single line of code</em>.
</li></ol><ol start="5"><li>There is no need to introduce a <code>_translate_for_sage_x_y</code>. It suffices when developers understand how different ways of serialisation in Python and Cython work. Moreover, adding such method to any (existing) class in Sage and for any future version of Sage would be an unacceptable burden upon the developers.
</li></ol><p>
Let me elaborate on some of the points above.
</p>
<p>
You said that it is a change of internal data structure and now you even name it a change in logic. But frankly, the simple addition of one attribute does not even qualify as a change of data structure, IMHO. You did not change the meaning of existing attributes, or did you? That's the point that I already tried to make in my previous posts.
</p>
<p>
What counts in Python is the question whether an attribute exists and what value it has. Your code works if the new attribute <code>__weights</code> exists and has either the value <code>None</code> or provides degree weights.
</p>
<p>
Thus, <em>any</em> way of providing the default value <code>None</code> for <code>__weights</code> when reading an old pickle would be just fine, without changing the main body of your code.
</p>
<p>
We are discussing three ways of providing the default value: <code>__setstate__</code>, <code>__getattr__</code> and a class attribute. Since the rest of your code won't change, it should be easy to implement each of the three approaches, and test how they perform.
</p>
<p>
My guess is that <code>__getattr__</code> would slow things down. <code>__setstate__</code> (your current solution) will probably be fine, performance-wise, but it is more code than providing a class attribute (which is just <em>one line of code</em>!).
</p>
<p>
But I want to see tests!
</p>
<blockquote class="citation">
<p>
Therefore the solution you suggest is not suitable to solve this problem.
</p>
</blockquote>
<p>
Did you try? I doubt that you did.
</p>
<blockquote class="citation">
<p>
Also in my point of view, even if it's possible, it would introduce lots of code into the main body of the <a class="missing wiki">TermOrder?</a> class, just to solve the unpickling problem of old objects!
</p>
</blockquote>
<p>
Lots of code???? We are talking about one single line!!
</p>
<p>
Namely:
Remove the <code>__setstate__</code> that you recently added. Instead, insert the line
</p>
<pre class="wiki"> __weights = None
</pre><p>
right after the doc string of the class. Then, <code>__weights</code> becomes a class attribute, and <code>None</code> will be available as the default value that you are using anyway. There will be no change whatsoever in the main code body!
</p>
<p>
To avoid misunderstanding: The line should <em>not</em> be inserted into the <code>__init__</code> method. This is since the <code>__init__</code> method will not be invoked at unpickling.
</p>
<p>
Apart from that: I doubt that there is any unpickling problem whose resolution would involve a change in the main body of code.
</p>
<p>
Namely: There are different ways of pickling. But each relies on well-localised things, for example methods: <code>__getinitargs__</code> or <code>__getstate__</code> or <code>__reduce__</code>; or it relies on the simple fact that Python saves <code>__dict__</code> (that's the case in your example).
</p>
<p>
Unpickling is local as well. It relies on <code>__init__</code> or <code>__setstate__</code> or an unpickling function.
</p>
<p>
Hence, the changes needed to solve any unpickling problem are local to these methods, with the only exception of <code>__dict__</code>. But this problem can be solved locally as well, as we have seen. QED.
</p>
<blockquote class="citation">
<p>
I believe using <span class="underline">setstate</span> method is the most simple and natural way to solve the problem.
</p>
</blockquote>
<p>
It is debatable whether a lengthy <code>__setstate__</code> method that internally constructs a temporary term order just for getting one default value is simpler and more natural than a single line of code.
</p>
<blockquote class="citation">
<p>
Actually, this is the way the Python documentation, which I quoted in the sage-dev thread, recommends to solve such problems.
</p>
</blockquote>
<p>
That would be the case for more difficult examples. But the addition of one attribute with default value <code>None</code> is not difficult.
</p>
<blockquote class="citation">
<p>
The translating method is automatically invoked by <span class="underline">setstate</span> method in the <a class="missing wiki">SageObject?</a> class when unpickling objects before Sage 4.7, perhaps with issuing a warning message to the user.
</p>
</blockquote>
<p>
There are different ways of pickling/unpickling. Not all of them involve a <code>__setstate__</code> method. And issuing a warning message is not an acceptable option.
</p>
<blockquote class="citation">
<p>
Another developer may add another translating method like "_translate_for_sage_4_8".
</p>
</blockquote>
<p>
You see the induction? 4_7, 4_8, 5_0, 5_1? That's not practical, and also not needed.
</p>
<blockquote class="citation">
<p>
I guess without such an infrastructure, developers will get discouraged to change internals of classes to enhance Sage, because of the inevitable unpickling problem.
</p>
</blockquote>
<p>
I really think you overestimate the difficulty of unpickling. And it will most certainly discourage developers if they suddenly have to maintain a <code>_translate_from_</code> method for any class they ever wrote.
</p>
TicketSimonKingMon, 30 May 2011 07:33:44 GMTpriority changed
https://trac.sagemath.org/ticket/11316#comment:19
https://trac.sagemath.org/ticket/11316#comment:19
<ul>
<li><strong>priority</strong>
changed from <em>minor</em> to <em>major</em>
</li>
</ul>
<p>
Here is a further way to solve the problem.
</p>
<p>
I noticed that you use a method <code>is_weighted_degree_order</code> for testing whether <code>__weights</code> is not None, and you consequently use it (hence, you do not have <code>if self.__weights is not None</code>) in other parts of your code. That is good!
</p>
<p>
If you have an old pickle then <code>__weights</code> is not present. Hence, <code>self.__weights</code> will result in an attribute error, and this error indicates that you have no weighted degree order.
</p>
<p>
Hence, the fourth suggestion is to remove <code>__setstate__</code>, and to simply rewrite <code>is_weighted_degree_order</code> as follows:
</p>
<pre class="wiki">def is_weighted_degree_order(self):
try:
return self.__weights is not None
except AttributeError:
self.__weights = None
return False
</pre><p>
That's another clean solution. There might be additional changes in your code needed, but I guess these changes would be minor.
</p>
<p>
I would like to see timings for all four solutions. Namely, it may be that frequent calls to functions like <code>is_weighted_degree_order</code> slows down arithmetic.
</p>
<p>
By the way: I think that the addition of weighted degree orders is no "minor" extension. Therefore I'm bumping up the priority of this ticket.
</p>
TicketSimonKingMon, 30 May 2011 19:48:18 GMT
https://trac.sagemath.org/ticket/11316#comment:20
https://trac.sagemath.org/ticket/11316#comment:20
<p>
In the third patch, all double underscore attributes are renamed into single underscore attributes -- including old attributes.
</p>
<p>
While the simple addition of one attribute is no substantial change of data structure, changing <code>__name</code> into <code>_name</code> etc certainly <em>is</em>. That undoubtedly makes unpickling more complicated, to the extent that introducing <code>_weights</code> as a class attribute would not suffice anymore. With that patch, there is probably no way around using <code>__setstate__</code>.
</p>
<p>
The good news is: A block order pickled with <code>sage-4.6.2</code> can be read with the third patch.
</p>
<p>
I think we agree that we don't like double underscore so much. The question is: Should our dislike be reason enough to change it, if the price to pay is an unpickling that could certainly be simpler without changing the old names? Or would that be shooting ourselves in the foot?
</p>
<p>
If we wish unpickling to be easy then the further work should be based on the second patch, with <code>__setstate__</code> removed or at least simplified (namely without creating a temporary term order). If we wish to get rid of double underscore attributes (including the old ones) then I guess there is no short elegant way of preserving backward compatibility.
</p>
<p>
I'd like to know the opinion of Martin and/or Burcin on that point.
</p>
TicketSimonKingMon, 30 May 2011 20:21:57 GMT
https://trac.sagemath.org/ticket/11316#comment:21
https://trac.sagemath.org/ticket/11316#comment:21
<p>
I just noticed that the second patch also changes old names: The old attribute <code>blocks</code> is renamed <code>__blocks</code> in the second patch, and there is a new method <code>blocks</code>. Hence, what used to be an attribute became a method.
</p>
<p>
Even worse: An old pickle may contain the old attribute <code>block</code>. When reading it, wouldn't that attribute <code>block</code> override the new method <code>block</code> (unless one has an appropriate <code>__setattr__</code>, of course)?
</p>
<p>
That said, I do think that <code>block</code> should better be the name of a method and not of a plain attribute.
</p>
<p>
And again, the question (addressed to all of you) is whether the reason for these changes is sufficient. There <em>is</em> a reason, but the changes make backward compatibility needlessly complicated IMHO.
</p>
TicketburcinMon, 30 May 2011 20:59:06 GMT
https://trac.sagemath.org/ticket/11316#comment:22
https://trac.sagemath.org/ticket/11316#comment:22
<p>
I haven't read the patch carefully, so I can't address any specific points, but I am OK with a major refactoring of the <code>TermOrder</code> class.
</p>
<p>
Of course, we still need to ensure that old pickles can be accessed without problems. I would be happier if this patch went in with at least a minor version change (for example 4.8) and got a mention for breaking backward compatibility (of the term order API) in the release notes.
</p>
TicketkleeTue, 31 May 2011 01:24:52 GMT
https://trac.sagemath.org/ticket/11316#comment:23
https://trac.sagemath.org/ticket/11316#comment:23
<p>
There is more change in the logic than mere adding one attribute, Simon. In the old <a class="missing wiki">TermOrder?</a>, the <code>__name</code> attribute essentially contains all information in string form about the term order. For example, <code>__name</code> is <code></code>lex(2),degrevlex(3)<code></code> for a block order. This is a convenient representation for Singular. In new <a class="missing wiki">TermOrder?</a>, <code>__name</code> is just <code></code>block<code></code> and <code>__blocks</code> contain <code>TermOrder(</code>lex<code>,2)</code> and <code>TermOrder(</code>degrevlex<code>,3)</code>. (I think the old <a class="missing wiki">TermOrder?</a> is somewhat Singular-centric, which may be justified since Sage heavily relies on Singular.) You could appreciate the amount of change in logic only after reading the old <a class="missing wiki">TermOrder?</a> class. Adding just <code>__weights=None</code> when unpickling old <code>TermOrder(</code><code>lex(2),degrevlex(3)</code>`) will result in unpickling failure. I hope Martin come up and confirm this.
</p>
<p>
I will not further insist on my proposal of the unpickling infrastructure. Perhaps the issue will rise again if that is really necessary.
</p>
<p>
The third patch applies cleanly to Sage 4.7 and there is no doctest failure. On my part, the patch is quite satisfactory and the unpickling problem is elegantly solved. :-) So I feel no need to spend more time on this.
</p>
<p>
As Burcin suggested, if this patch would be merged to Sage, we have to mention for breaking backward compatibility (by removing <code>block</code> attribute) in the release notes.
</p>
TicketSimonKingTue, 31 May 2011 07:53:14 GMT
https://trac.sagemath.org/ticket/11316#comment:24
https://trac.sagemath.org/ticket/11316#comment:24
<p>
Hi Kwankyu,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11316#comment:23" title="Comment 23">klee</a>:
</p>
<blockquote class="citation">
<p>
There is more change in the logic than mere adding one attribute, Simon.
</p>
</blockquote>
<p>
Yep, meanwhile I noticed it (see my previous post), and also I got more sleep.
</p>
<blockquote class="citation">
<p>
Adding just <code>__weights=None</code> when unpickling old <code>TermOrder(</code><code>lex(2),degrevlex(3)</code>`) will result in unpickling failure. I hope Martin come up and confirm this.
</p>
</blockquote>
<p>
I can confirm it as well.
</p>
<p>
I found that adding <code>_weights=None</code> and <code>_blocks=()</code> made it possible to read old pickles even without <code>__setstate__</code>, but then the semantical change of <code>_name</code> and <code>blocks</code> struck.
</p>
<blockquote class="citation">
<p>
The third patch applies cleanly to Sage 4.7 and there is no doctest failure. On my part, the patch is quite satisfactory and the unpickling problem is elegantly solved. :-) So I feel no need to spend more time on this.
</p>
</blockquote>
<p>
I still find it awkward to create a temporary term order in <code>__setstate__</code>. However, it works.
</p>
<blockquote class="citation">
<p>
As Burcin suggested, if this patch would be merged to Sage, we have to mention for breaking backward compatibility (by removing <code>block</code> attribute) in the release notes.
</p>
</blockquote>
<p>
In the release notes and also in the documentation.
</p>
<p>
There would be the possibility to find a different name than <code>blocks()</code> in order to avoid a name conflict with an old attribute. But I think <code>blocks()</code> is a better name than, e.g., <code>block_list()</code>.
</p>
<p>
So, for now, I have no objections against the third patch version. I'll probably give it a positive review after reading it thoroughly (including the documentation) and running the doctests.
</p>
<p>
For the patchbot:
</p>
<p>
Apply trac_11316.3.patch
</p>
TicketSimonKingTue, 31 May 2011 15:47:40 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/11316#comment:25
https://trac.sagemath.org/ticket/11316#comment:25
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>Singular conversion with negative degree weights</em>
</li>
</ul>
<p>
I am checking the documentation, and tried to add some more examples that expose the use of degree weights, and I found one problem.
</p>
<p>
In the documentation, there is no statement on what values are allowed as degree weights. So, I assume that any real number is allowed (but this should be stated in the docs as well). In particular, negative degree weights should be allowed (this is something that I use in my current project).
</p>
<p>
So, I tested
</p>
<pre class="wiki">sage: N.<a,b,c> = PolynomialRing(QQ, 3, order=TermOrder('wdeglex',[-1,2,-3]))
sage: c<a^2<1
True
</pre><p>
which is correct.
</p>
<p>
But the conversion to Singular fails:
</p>
<pre class="wiki">sage: T = N.term_order()
sage: T.singular_str() # this is correct as well
'Wp(-1,2,-3)'
sage: singular(N)
---------------------------------------------------------------------------
RuntimeError Traceback (most recent call last)
/mnt/local/king/SAGE/sage-4.7.rc2/devel/sage-main/<ipython console> in <module>()
/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/sage/interfaces/singular.pyc in __call__(self, x, type)
653 return self(x.sage())
654 elif not isinstance(x, ExpectElement) and hasattr(x, '_singular_'):
--> 655 return x._singular_(self)
656
657 # some convenient conversions
/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/sage/rings/polynomial/multi_polynomial_libsingular.so in sage.rings.polynomial.multi_polynomial_libsingular.MPolynomialRing_libsingular._singular_ (sage/rings/polynomial/multi_polynomial_libsingular.cpp:9291)()
/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/sage/rings/polynomial/multi_polynomial_libsingular.so in sage.rings.polynomial.multi_polynomial_libsingular.MPolynomialRing_libsingular._singular_init_ (sage/rings/polynomial/multi_polynomial_libsingular.cpp:9851)()
/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/sage/interfaces/singular.pyc in ring(self, char, vars, order, check)
903 s = '; '.join(['if(defined(%s)>0){kill %s;};'%(x,x)
904 for x in vars[1:-1].split(',')])
--> 905 self.eval(s)
906
907 if check and isinstance(char, (int, long, sage.rings.integer.Integer)):
/mnt/local/king/SAGE/sage-4.7.rc2/local/lib/python2.6/site-packages/sage/interfaces/singular.pyc in eval(self, x, allow_semicolon, strip, **kwds)
548
549 if s.find("error") != -1 or s.find("Segment fault") != -1:
--> 550 raise RuntimeError, 'Singular error:\n%s'%s
551
552 if get_verbose() > 0:
RuntimeError: Singular error:
? `a` is not defined
? error occurred in or before STDIN line 25: `ill 0*a;};; if(defined( 0*b)>0){kill 0*b;};; if(defined( 0*c)>0){kill 0*c;};`
? `b` is not defined
? error occurred in or before STDIN line 25: `ill 0*a;};; if(defined( 0*b)>0){kill 0*b;};; if(defined( 0*c)>0){kill 0*c;};`
? `c` is not defined
? error occurred in or before STDIN line 25: `ill 0*a;};; if(defined( 0*b)>0){kill 0*b;};; if(defined( 0*c)>0){kill 0*c;};`
</pre><p>
I assume that the example <em>should</em> work. And in Singular, it <em>does</em> work:
</p>
<pre class="wiki">
SINGULAR / Development
A Computer Algebra System for Polynomial Computations / version 3-1-1
0<
by: G.-M. Greuel, G. Pfister, H. Schoenemann \ Feb 2010
FB Mathematik der Universitaet, D-67653 Kaiserslautern \
> ring r = 0,(a,b,c),Wp(-1,2,-3);
> r;
// characteristic : 0
// number of vars : 3
// block 1 : ordering Wp
// : names a b c
// : weights -1 2 -3
// block 2 : ordering C
</pre><p>
So, I'm putting it as "needs work".
</p>
TicketSimonKingTue, 31 May 2011 15:59:14 GMTstatus changed; work_issues deleted
https://trac.sagemath.org/ticket/11316#comment:26
https://trac.sagemath.org/ticket/11316#comment:26
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>work_issues</strong>
<em>Singular conversion with negative degree weights</em> deleted
</li>
</ul>
<p>
... and the reason for the error is that the the string representation of polynomials does not correctly deal with polynomials of negative degree:
</p>
<pre class="wiki">sage: str(N.gens())
'(0*a, 0*b, 0*c)'
</pre><p>
That should be '(a, b, c)'.
</p>
<p>
Looking at <code>a._repr_</code>, I see that in fact Singular is to blame. And indeed, in Singular we have
</p>
<pre class="wiki">> ring r = 0,(a,b,c),Wp(-1,2,-3);
> b;
0b
> ring r = 0,(a,b,c),Wp(1,2,0);
// ** redefining r **
> b;
0b
</pre><p>
So, there is a bug in Singular that should be reported upstream.
</p>
<p>
Nevertheless, I think it should be addressed here, namely by stating in the documentation that the weights have to be positive integral. I can do that in a reviewer patch, if you like.
</p>
TicketSimonKingTue, 31 May 2011 16:01:51 GMT
https://trac.sagemath.org/ticket/11316#comment:27
https://trac.sagemath.org/ticket/11316#comment:27
<p>
Singular states in its documentation that <a class="ext-link" href="http://www.singular.uni-kl.de/Manual/latest/sing_743.htm#IDX289"><span class="icon"></span>the degrees for wp need to be positive integral</a>. So, they may not acknowledge that it is a bug. But I will try.
</p>
TicketSimonKingTue, 31 May 2011 16:09:45 GMT
https://trac.sagemath.org/ticket/11316#comment:28
https://trac.sagemath.org/ticket/11316#comment:28
<p>
Or it can be solved on the side of Sage. Namely, while Singular does not allow the weighted order <code>wp(1,2,0)</code>, it <em>does</em> allow to provide the <code>dp</code> order with an additional weight vector, and this weight vector can contain negative numbers:
</p>
<pre class="wiki">> ring r = 0,(a,b,c),(a(1,-2,0),dp);
> a,b,c; // so, printing in Singular is correct
a b c
> deg(b); // and the given degrees are used
-2
> deg(c);
0
</pre><p>
Couldn't we change the method <code>TermOrder.singular_str()</code>, such that <code>(a(...),dp)</code> is used if there is any non-positive weight?
</p>
TicketkleeWed, 01 Jun 2011 00:49:00 GMT
https://trac.sagemath.org/ticket/11316#comment:29
https://trac.sagemath.org/ticket/11316#comment:29
<p>
<code>wp</code>, that is, <code>wdegrevlex</code> term order is supposed to define so-called local term order in Sage as well as in Singular. Therefore negative integers are not allowed in weights.
</p>
<p>
Your term order <code>(a(...),dp)</code> is not a term order officially supported in Sage. In that case, Sage allows to force the term order by setting <code>force=True</code> So you could do
</p>
<pre class="wiki">sage: t = TermOrder('a(1,-2,0),dp',n=3,force=True)
sage: t
a(1,-2,0),dp term order
sage: t.singular_str()
'a(1,-2,0),dp'
</pre><p>
But this reveals a bug in dealing with the <code>force</code> argument in the current patch. So I prepared a fourth patch to fix this bug. I will shortly upload the fourth patch, after doctesting. Then you can experiment.
</p>
<p>
You are welcome to add more documentation in the reviewer patch.
</p>
TicketkleeWed, 01 Jun 2011 00:49:50 GMTattachment set
https://trac.sagemath.org/ticket/11316
https://trac.sagemath.org/ticket/11316
<ul>
<li><strong>attachment</strong>
set to <em>trac_11316.4.patch</em>
</li>
</ul>
<p>
fixed a bug in dealing with the force argument
</p>
TicketkleeWed, 01 Jun 2011 01:12:29 GMT
https://trac.sagemath.org/ticket/11316#comment:30
https://trac.sagemath.org/ticket/11316#comment:30
<p>
I correct my mistake:
</p>
<p>
<code>wp</code>, that is, <code>wdegrevlex</code> term order is supposed to define so-called global term order in Sage as well as Singular. Therefore negative integers are not allowed in weights.
</p>
TicketSimonKingWed, 01 Jun 2011 05:51:33 GMT
https://trac.sagemath.org/ticket/11316#comment:31
https://trac.sagemath.org/ticket/11316#comment:31
<p>
With the fourth patch, it is still allowed to do
</p>
<pre class="wiki">sage: N.<a,b,c> = PolynomialRing(QQ, 3, order=TermOrder('wdeglex',[0,-2,3]))
</pre><p>
There should an error be raised, because negative degrees do not work at all, due to the limitations imposed by Singular:
</p>
<pre class="wiki">sage: a
0*a
</pre><p>
Another example:
</p>
<pre class="wiki">sage: N.<a,b,c> = PolynomialRing(QQ, 3, order=TermOrder('wdeglex',[1.1,2,3]))
sage: a.degree()
1
</pre><p>
Hence, one may think that the degree is converted to an integer. But in fact the "broken" value of the degree is still hanging around:
</p>
<pre class="wiki">sage: singular(N)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
...
TypeError: Singular error:
? no ring active
? error occurred in or before STDIN line 27: `ring sage7=0,(a, b, c),Wp(1.10000000000000,2,3);`
? expected ring-expression. type 'help ring;'
</pre><p>
I am not sure what to do in the second case: Raise an error, or silently convert <code>1.1</code> to <code>1</code>?
</p>
TicketSimonKingWed, 01 Jun 2011 15:11:46 GMTattachment set
https://trac.sagemath.org/ticket/11316
https://trac.sagemath.org/ticket/11316
<ul>
<li><strong>attachment</strong>
set to <em>trac11316_reviewer.patch</em>
</li>
</ul>
<p>
Raise an error on invalid degree weights. State in the docs which weights are accepted
</p>
TicketSimonKingWed, 01 Jun 2011 15:21:53 GMTstatus, description changed; reviewer set
https://trac.sagemath.org/ticket/11316#comment:32
https://trac.sagemath.org/ticket/11316#comment:32
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Simon King</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/11316?action=diff&version=32">diff</a>)
</li>
</ul>
<p>
I was reading the patch, and it seems ok. Meanwhile I think that <code>__setstate__</code> is the best way to solve the unpickling problem for old pickles, given the fact that both name and meaning of several attributes have changed. I can confirm that old pickles can be correctly unpickled, including a block order (the old attribute <code>blocks</code> does not override the new method <code>blocks()</code>).
</p>
<p>
Having weighted degree orders is a good thing. There are some limitations inherited from Singular: The degree weights have to be positive integers.
</p>
<p>
I give a positive review, and add a reviewer patch, which I hope is fine for you.
</p>
<p>
The reviewer patch adds to the docs that the degree weights must be positive integers, and it lets an error be raised if any weight is non-positive. It is attempted to convert any non-integral weight to an integer. In particular, a weight such as <code>1.1</code> will silently be converted into <code>1</code>. There are doctests for that behaviour.
</p>
<p>
Also, I add a comment in the doc of the new method <code>blocks()</code>, stating that back in the old days some orders had an attribute of the same name, so that we have a backward incompatible (but apparently not problematic) change.
</p>
<p>
So, if you don't oppose against my reviewer patch:
</p>
<p>
Apply trac_11316.4.patch trac11316_reviewer.patch
</p>
TicketSimonKingMon, 06 Jun 2011 11:42:44 GMT
https://trac.sagemath.org/ticket/11316#comment:33
https://trac.sagemath.org/ticket/11316#comment:33
<p>
Note a related ticket: <a class="closed ticket" href="https://trac.sagemath.org/ticket/11431" title="defect: Conversion from Singular to Sage (closed: fixed)">#11431</a>, which extends the capabilities of conversion from Singular to Sage. It makes use of weighted/matrix/block term orders. So, thank you for implementing it!
</p>
TicketjdemeyerWed, 08 Jun 2011 20:54:51 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/11316#comment:34
https://trac.sagemath.org/ticket/11316#comment:34
<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-4.7.1.alpha3</em>
</li>
</ul>
TicketjdemeyerTue, 14 Jun 2011 19:36:48 GMTstatus changed; resolution, merged deleted
https://trac.sagemath.org/ticket/11316#comment:35
https://trac.sagemath.org/ticket/11316#comment:35
<ul>
<li><strong>status</strong>
changed from <em>closed</em> to <em>new</em>
</li>
<li><strong>resolution</strong>
<em>fixed</em> deleted
</li>
<li><strong>merged</strong>
<em>sage-4.7.1.alpha3</em> deleted
</li>
</ul>
<p>
There are some issues on Solaris. On the "marks" and "t2" buildbots, I get the following error (identical on both systems):
</p>
<pre class="wiki">sage -t -long -force_lib devel/sage/sage/rings/polynomial/term_order.py
**********************************************************************
File "/scratch/buildbot/sage/t2-1/t2_full/build/sage-4.7.1.alpha3/devel/sage-main/sage/rings/polynomial/term_order.py", line 1878:
sage: singular.eval('ring r1 = (9,x),(a,b,c,d,e,f),(M((1,2,3,0)),wp(2,3),lp)')
Exception raised:
Traceback (most recent call last):
File "/scratch/buildbot/sage/t2-1/t2_full/build/sage-4.7.1.alpha3/local/bin/ncadoctest.py", line 1231, in run_one_test
self.run_one_example(test, example, filename, compileflags)
File "/scratch/buildbot/sage/t2-1/t2_full/build/sage-4.7.1.alpha3/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
File "/scratch/buildbot/sage/t2-1/t2_full/build/sage-4.7.1.alpha3/local/bin/ncadoctest.py", line 1172, in run_one_example
compileflags, 1) in test.globs
File "<doctest __main__.example_64[2]>", line 1, in <module>
singular.eval('ring r1 = (9,x),(a,b,c,d,e,f),(M((1,2,3,0)),wp(2,3),lp)')###line 1878:
sage: singular.eval('ring r1 = (9,x),(a,b,c,d,e,f),(M((1,2,3,0)),wp(2,3),lp)')
File "/scratch/buildbot/sage/t2-1/t2_full/build/sage-4.7.1.alpha3/local/lib/python/site-packages/sage/interfaces/singular.py", line 567, in eval
raise RuntimeError, 'Singular error:\n%s'%s
RuntimeError: Singular error:
? cannot open `gftables/9`
? cannot make ring
? error occurred in or before STDIN line 33: `ring r1 = (9,x),(a,b,c,d,e,f),(M((1,2,3,0)),wp(2,3),lp);`
? expected ring-expression. type 'help ring;'
**********************************************************************
File "/scratch/buildbot/sage/t2-1/t2_full/build/sage-4.7.1.alpha3/devel/sage-main/sage/rings/polynomial/term_order.py", line 1881:
sage: termorder_from_singular(singular)
Expected:
Block term order with blocks:
(Matrix term order with matrix
[1 2]
[3 0],
Weighted degree reverse lexicographic term order with weights (2, 3),
Lexicographic term order of length 2)
Got:
Block term order with blocks:
(Lexicographic term order of length 3,
Degree lexicographic term order of length 5,
Lexicographic term order of length 2)
**********************************************************************
</pre>
TicketSimonKingWed, 15 Jun 2011 06:20:14 GMTstatus changed
https://trac.sagemath.org/ticket/11316#comment:36
https://trac.sagemath.org/ticket/11316#comment:36
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketSimonKingWed, 15 Jun 2011 06:23:01 GMTstatus changed
https://trac.sagemath.org/ticket/11316#comment:37
https://trac.sagemath.org/ticket/11316#comment:37
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Hi Jeroen,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11316#comment:35" title="Comment 35">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
There are some issues on Solaris. On the "marks" and "t2" buildbots, I get the following error (identical on both systems):
File "/scratch/buildbot/sage/t2-1/t2_full/build/sage-4.7.1.alpha3/devel/sage-main/sage/rings/polynomial/term_order.py", line 1878:
</p>
<blockquote>
<p>
sage: singular.eval('ring r1 = (9,x),(a,b,c,d,e,f),(M((1,2,3,0)),wp(2,3),lp)')
</p>
</blockquote>
</blockquote>
<p>
That test has only been introduced by <a class="closed ticket" href="https://trac.sagemath.org/ticket/11431" title="defect: Conversion from Singular to Sage (closed: fixed)">#11431</a>. Hence, I am reverting the ticket here into "positive review"
</p>
TicketSimonKingWed, 15 Jun 2011 06:35:04 GMTstatus changed
https://trac.sagemath.org/ticket/11316#comment:38
https://trac.sagemath.org/ticket/11316#comment:38
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_info</em>
</li>
</ul>
<p>
Or perhaps I was too quick. After all, one of the patches changes sage/libs/singular/ring.pyx, and it is imaginable that this has an effect on singular.
</p>
<p>
I have no recent working Sage version on my t2 account. Could you please test whether the line
</p>
<pre class="wiki">sage: singular.eval('ring r1 = (9,x),(a,b,c,d,e,f),(M((1,2,3,0)),wp(2,3),lp)')
</pre><p>
works without and with the patch applied?
</p>
TicketjdemeyerWed, 15 Jun 2011 07:26:19 GMT
https://trac.sagemath.org/ticket/11316#comment:39
https://trac.sagemath.org/ticket/11316#comment:39
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11316#comment:38" title="Comment 38">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Or perhaps I was too quick. After all, one of the patches changes sage/libs/singular/ring.pyx, and it is imaginable that this has an effect on singular.
</p>
<p>
I have no recent working Sage version on my t2 account.
</p>
</blockquote>
<p>
I have no account on <code>t2</code> but I could test on <code>mark2</code> instead (but this will take a while, that is a very slow machine).
</p>
TicketjdemeyerWed, 15 Jun 2011 16:02:24 GMTcc changed
https://trac.sagemath.org/ticket/11316#comment:40
https://trac.sagemath.org/ticket/11316#comment:40
<ul>
<li><strong>cc</strong>
<em>mpatel</em> added
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11316#comment:39" title="Comment 39">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11316#comment:38" title="Comment 38">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Or perhaps I was too quick. After all, one of the patches changes sage/libs/singular/ring.pyx, and it is imaginable that this has an effect on singular.
</p>
<p>
I have no recent working Sage version on my t2 account.
</p>
</blockquote>
<p>
I have no account on <code>t2</code> but I could test on <code>mark2</code> instead (but this will take a while, that is a very slow machine).
</p>
</blockquote>
<p>
I am not able to build Sage on <code>mark2</code> (GNUTLS fails). I don't know how the buildbot manages.
</p>
TicketSimonKingTue, 02 Aug 2011 11:32:23 GMT
https://trac.sagemath.org/ticket/11316#comment:41
https://trac.sagemath.org/ticket/11316#comment:41
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/11316#comment:40" title="Comment 40">jdemeyer</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
I have no account on <code>t2</code> but I could test on <code>mark2</code> instead (but this will take a while, that is a very slow machine).
</p>
</blockquote>
<p>
I am not able to build Sage on <code>mark2</code> (GNUTLS fails). I don't know how the buildbot manages.
</p>
</blockquote>
<p>
Too bad. I am now trying to build on mark -- no idea whether it will work, of course.
</p>
<p>
One question: Wouldn't it be possible to use the Sage version that was built by the buildbot?
</p>
TicketSimonKingWed, 03 Aug 2011 18:33:50 GMTstatus changed
https://trac.sagemath.org/ticket/11316#comment:42
https://trac.sagemath.org/ticket/11316#comment:42
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
</ul>
<p>
As much as I understand, the ticket here is independent from <a class="closed ticket" href="https://trac.sagemath.org/ticket/11431" title="defect: Conversion from Singular to Sage (closed: fixed)">#11431</a>, right?
</p>
<p>
It turned out that the Solaris problem pointed out by Jeroen exists in a fresh sage-4.7.1.rc1 installation on mark. In particular, the problem was definitely not introduced by the patch here.
</p>
TicketSimonKingWed, 03 Aug 2011 18:34:24 GMTstatus changed
https://trac.sagemath.org/ticket/11316#comment:43
https://trac.sagemath.org/ticket/11316#comment:43
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
For the reason pointed out in my previous post, I think it is alright to reinstate the positive review.
</p>
TicketjdemeyerFri, 05 Aug 2011 09:03:41 GMTmilestone changed
https://trac.sagemath.org/ticket/11316#comment:44
https://trac.sagemath.org/ticket/11316#comment:44
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.7.1</em> to <em>sage-4.7.2</em>
</li>
</ul>
TicketjdemeyerTue, 16 Aug 2011 09:26:03 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/11316#comment:45
https://trac.sagemath.org/ticket/11316#comment:45
<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-4.7.2.alpha1</em>
</li>
</ul>
Ticket