Weighted degree term orders added

[kwankyu]

Weighted degree term orders (wp,Wp,ws,Ws as in Singular) added to TermOrder.

New term orders as well as matrix term orders can be used in block term orders.

Apply:

1. attachment: trac_11316.4.patch

2. attachment: trac11316_reviewer.patch

CC: @burcin @qed777

Component: basic arithmetic

Author: Kwankyu Lee

Reviewer: Simon King

Merged: sage-4.7.2.alpha1

Issue created by migration from https://trac.sagemath.org/ticket/11316

[kwankyu]

Author: Kwankyu Lee

[kwankyu]

comment:1

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 TermOrder is now a method. Use

t.blocks()

instead of

t.blocks

Perhaps this change will require a deprecation warning. I am not sure how to do this for attributes.

[kwankyu]

comment:3

Regarding to converting "blocks" to "blocks()", Jason remarks:

To my understanding, we prefer methods to properties because:

1. Methods have docstrings and can be introspected (using question mark) to obtain the docs.

2. We want to be consistent, as it can be confusing for a user if some things require parentheses (methods) while others don't (properties).

I really wish we could use properties more, as it is more pythonic and looks cleaner, but those are two good points above...

[malb]

comment:4

• the patch does not apply cleanly against 4.7.rc2, but the failure is minor: only an update of a docstring in pbori.pyx fails.
  • Doctests do not pass for 4.7.rc2:
  • pbori.pyx I didn't fix the patch to apply cleanly, hence was to be expected that this one fails.
  • sage_object.pyx the patch seems to break pickling

So, the only real issue seems to be pickling.

[kwankyu]

comment:5

I rebased the patch on 4.7.rc2. pbori.pyx is no longer a problem.

I am not familiar with pickling. As I understand it, the patch modified the data structure of TermOrder objects in such an incompatible way that the old pickled TermOrder 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.

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

https://github.com/sagemath/sage-prod/issues/10768

I change the status of this ticket to "needs review" to get attention. Is "needs info" the right status?

[malb]

comment:6

Replying to @kwankyu:

I am not familiar with pickling. As I understand it, the patch modified the data structure of TermOrder objects in such an incompatible way that the old pickled TermOrder 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.

Your technical description seems right. However, in Sage's convention it is considered a bug if the pickle jar breaks.

I don't know what to do with this. A quick way to fix this is just to update the "pickle jar" of Sage

This is not what is supposed to happen. The point of the pickle jar is to ensure old objects can be unpickled.

, and, perhaps, this is up to the release manager, and perhaps a consensus in sage- devel is required. My reference is

https://github.com/sagemath/sage-prod/issues/10768

I change the status of this ticket to "needs review" to get attention. Is "needs info" the right status?

needs info or needs work are both fine as far as I understand it.

You can start debugging it by calling

sage.structure.sage_object.unpickle_all("YOUR_SAGE_ROOT/data/extcode/pickle_jar/pickle_jar.tar.bz2")

I'll also try to take a look soon-ish.

[kwankyu]

Attachment: trac_11316.patch.gz

[kwankyu]

comment:7

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.

[malb]

comment:8

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 __weights to become _weights. 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.

[malb]

comment:10

Perhaps, this is a good way of dealing with this:

def __getattr__(self,'name'):
    if name == "_weights": # I don't think it works for double underscore attributes?
        self._weights = None
        return self._weights
    else:
        raise AttributeError("'TermOrder' has no attribute '%s'"%name)

[kwankyu]

Attachment: trac_11316.2.patch.gz

fixed the unpickling failures

[kwankyu]

comment:11

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.

[kwankyu]

comment:12

For record,

https://groups.google.com/forum/#!topic/sage-devel/z2r5ag-j1WY

[malb]

comment:13

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.

[simon-king-jena]

comment:14

Replying to @kwankyu:

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.

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 for which it was originally defined.

By consequence, there is a problem for subclasses:

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'

So, surprise: __n seems gone. But it is there, under a different name:

sage: b._A__n
25

It is obvious that, under these circumstances, single underscore names are easier to deal with.

Remark: No mangling occurs if the name ends with two underscores. That's why all the magical methods __repr__, __add__ etc. can be easily inherited.

[simon-king-jena]

comment:15

Concerning unpickling of old pickles, I think the following holds in this case:

The pickling of a TermOrder is actually quite simple, standard Python. It is an object with a __dict__, and a new un-initialised instance can be created with __new__.

Example

First, we obtain an order:

sage: P.<x,y>=QQ[]
sage: T = P.term_order()

Now, if I am not mistaken, S=loads(dumps(T)) essentially boils down to the following. First, we create a new instance of the class:

sage: S = T.__new__(T.__class__)

And then, we provide the new instance with a copy of the old instance's __dict__:

sage: S.__dict__ = copy(T.__dict__)

As a result, we have a copy of T:

  sage: S
  Degree reverse lexicographic term order
  sage: S == T
  True
  sage: S is T
  False

If I understand correctly, the problem here is that your patch introduces a new attribute _weights, that has a default value for trivial degree weights. So, if __dict__ was taken from an old pickle, then it lacks the key '_weights'. Hence, requesting S._weights would fail.

Two solutions were suggested:

1. Introduce a __getattr__ method, that returns the default weights and puts them into S.__dict__['_weights']. That ensures that S.__getattr__('_weights') is called at most once: __getattr__ is only called if an attribute can not be found in the __dict__ and not as a class attribute.

2. Introduce the default _weights as a class attribute. If a term order has non-default weights then they are put into __dict__ (this is what happens if you do T._weights = [1,2,3]). But attributes found in __dict__ have precedence over class attributes. Hence, the class attribute will only be considered if _weights is not in __dict__.

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 __getattr__ may slow things down. How often are attributes of a term order called when you do polynomial arithmetic?

[simon-king-jena]

comment:16

I was reading part of the second patch. So, this is only part of a review.

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.

Certainly, one way (not necessarily the easiest way) to solve the unpickling problem is to write a __setstate__ method. That is what you do in the second patch. However, it seems awkward to create a whole new termorder inside __setstate__ (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 __weights) directly?

In addition, it seems to me that one does not need to introduce a __setstate__ method in order to solve the unpickling problems. Introducing a class attribute _weights (resp. __weights, 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 __weights should be modified so that the case __weights==None is correctly dealt with.

[kwankyu]

comment:17

To Simon,

I should say, I also don't like double underscored attributes. I just inherited them from previous authors of the class.

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.

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 TermOrder class, just to solve the unpickling problem of old objects!

I believe using __setstate__ 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.

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 __setstate__ method in the SageObject 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.

I guess without such an infrastructure, developers will get discouraged to change internals of classes to enhance Sage, because of the inevitable unpickling problem.

By the way, it is easy to change the double underscored attributes into single underscored ones with __setstate__ method.

[kwankyu]

rebased to Sage 4.7; replaced double underscored attributes to single underscored ones

[simon-king-jena]

comment:18

Attachment: trac_11316.3.patch.gz

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.

To sum it up:

1. 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.

2. Breaking the pickle jar for the sake of the trivial addition of an attribute is a no-go.

3. As much as I see, absolutely any unpickling problem can be solved without to change the main code body.

4. Here, the unpickling problem could be solved by replacing your __setstate__ method with a single line of code.

5. There is no need to introduce a _translate_for_sage_x_y. 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.

Let me elaborate on some of the points above.

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.

What counts in Python is the question whether an attribute exists and what value it has. Your code works if the new attribute __weights exists and has either the value None or provides degree weights.

Thus, any way of providing the default value None for __weights when reading an old pickle would be just fine, without changing the main body of your code.

We are discussing three ways of providing the default value: __setstate__, __getattr__ 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.

My guess is that __getattr__ would slow things down. __setstate__ (your current solution) will probably be fine, performance-wise, but it is more code than providing a class attribute (which is just one line of code!).

But I want to see tests!

Therefore the solution you suggest is not suitable to solve this problem.

Did you try? I doubt that you did.

Also in my point of view, even if it's possible, it would introduce lots of code into the main body of the TermOrder class, just to solve the unpickling problem of old objects!

Lots of code???? We are talking about one single line!!

Namely: Remove the __setstate__ that you recently added. Instead, insert the line

    __weights = None

right after the doc string of the class. Then, __weights becomes a class attribute, and None will be available as the default value that you are using anyway. There will be no change whatsoever in the main code body!

To avoid misunderstanding: The line should not be inserted into the __init__ method. This is since the __init__ method will not be invoked at unpickling.

Apart from that: I doubt that there is any unpickling problem whose resolution would involve a change in the main body of code.

Namely: There are different ways of pickling. But each relies on well-localised things, for example methods: __getinitargs__ or __getstate__ or __reduce__; or it relies on the simple fact that Python saves __dict__ (that's the case in your example).

Unpickling is local as well. It relies on __init__ or __setstate__ or an unpickling function.

Hence, the changes needed to solve any unpickling problem are local to these methods, with the only exception of __dict__. But this problem can be solved locally as well, as we have seen. QED.

I believe using __setstate__ method is the most simple and natural way to solve the problem.

It is debatable whether a lengthy __setstate__ 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.

Actually, this is the way the Python documentation, which I quoted in the sage-dev thread, recommends to solve such problems.

That would be the case for more difficult examples. But the addition of one attribute with default value None is not difficult.

The translating method is automatically invoked by __setstate__ method in the SageObject class when unpickling objects before Sage 4.7, perhaps with issuing a warning message to the user.

There are different ways of pickling/unpickling. Not all of them involve a __setstate__ method. And issuing a warning message is not an acceptable option.

Another developer may add another translating method like "_translate_for_sage_4_8".

You see the induction? 4_7, 4_8, 5_0, 5_1? That's not practical, and also not needed.

I guess without such an infrastructure, developers will get discouraged to change internals of classes to enhance Sage, because of the inevitable unpickling problem.

I really think you overestimate the difficulty of unpickling. And it will most certainly discourage developers if they suddenly have to maintain a _translate_from_ method for any class they ever wrote.

[simon-king-jena]

comment:19

Here is a further way to solve the problem.

I noticed that you use a method is_weighted_degree_order for testing whether __weights is not None, and you consequently use it (hence, you do not have if self.__weights is not None) in other parts of your code. That is good!

If you have an old pickle then __weights is not present. Hence, self.__weights will result in an attribute error, and this error indicates that you have no weighted degree order.

Hence, the fourth suggestion is to remove __setstate__, and to simply rewrite is_weighted_degree_order as follows:

def is_weighted_degree_order(self):
    try:
        return self.__weights is not None
    except AttributeError:
        self.__weights = None
        return False

That's another clean solution. There might be additional changes in your code needed, but I guess these changes would be minor.

I would like to see timings for all four solutions. Namely, it may be that frequent calls to functions like is_weighted_degree_order slows down arithmetic.

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.

[simon-king-jena]

comment:20

In the third patch, all double underscore attributes are renamed into single underscore attributes -- including old attributes.

While the simple addition of one attribute is no substantial change of data structure, changing __name into _name etc certainly is. That undoubtedly makes unpickling more complicated, to the extent that introducing _weights as a class attribute would not suffice anymore. With that patch, there is probably no way around using __setstate__.

The good news is: A block order pickled with sage-4.6.2 can be read with the third patch.

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?

If we wish unpickling to be easy then the further work should be based on the second patch, with __setstate__ 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.

I'd like to know the opinion of Martin and/or Burcin on that point.

[simon-king-jena]

comment:21

I just noticed that the second patch also changes old names: The old attribute blocks is renamed __blocks in the second patch, and there is a new method blocks. Hence, what used to be an attribute became a method.

Even worse: An old pickle may contain the old attribute block. When reading it, wouldn't that attribute block override the new method block (unless one has an appropriate __setattr__, of course)?

That said, I do think that block should better be the name of a method and not of a plain attribute.

And again, the question (addressed to all of you) is whether the reason for these changes is sufficient. There is a reason, but the changes make backward compatibility needlessly complicated IMHO.

[burcin]

comment:22

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 TermOrder class.

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.

[kwankyu]

comment:23

There is more change in the logic than mere adding one attribute, Simon. In the old TermOrder, the __name attribute essentially contains all information in string form about the term order. For example, __name is lex(2),degrevlex(3) for a block order. This is a convenient representation for Singular. In new TermOrder, __name is just block and __blocks contain TermOrder(lex,2) and TermOrder(degrevlex,3). (I think the old TermOrder 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 TermOrder class. Adding just __weights=None when unpickling old `TermOrder(lex(2),degrevlex(3)) will result in unpickling failure. I hope Martin come up and confirm this.

I will not further insist on my proposal of the unpickling infrastructure. Perhaps the issue will rise again if that is really necessary.

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.

As Burcin suggested, if this patch would be merged to Sage, we have to mention for breaking backward compatibility (by removing block attribute) in the release notes.

[simon-king-jena]

comment:24

Hi Kwankyu,

Replying to @kwankyu:

There is more change in the logic than mere adding one attribute, Simon.

Yep, meanwhile I noticed it (see my previous post), and also I got more sleep.

Adding just __weights=None when unpickling old `TermOrder(lex(2),degrevlex(3)) will result in unpickling failure. I hope Martin come up and confirm this.

I can confirm it as well.

I found that adding _weights=None and _blocks=() made it possible to read old pickles even without __setstate__, but then the semantical change of _name and blocks struck.

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.

I still find it awkward to create a temporary term order in __setstate__. However, it works.

As Burcin suggested, if this patch would be merged to Sage, we have to mention for breaking backward compatibility (by removing block attribute) in the release notes.

In the release notes and also in the documentation.

There would be the possibility to find a different name than blocks() in order to avoid a name conflict with an old attribute. But I think blocks() is a better name than, e.g., block_list().

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.

For the patchbot:

Apply trac_11316.3.patch

[simon-king-jena]

Work Issues: Singular conversion with negative degree weights

[simon-king-jena]

comment:25

I am checking the documentation, and tried to add some more examples that expose the use of degree weights, and I found one problem.

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).

So, I tested

sage: N.<a,b,c> = PolynomialRing(QQ, 3, order=TermOrder('wdeglex',[-1,2,-3]))
sage: c<a^2<1
True

which is correct.

But the conversion to Singular fails:

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;};`

I assume that the example should work. And in Singular, it does work:

                     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

So, I'm putting it as "needs work".

[simon-king-jena]

Changed work issues from Singular conversion with negative degree weights to none

[simon-king-jena]

comment:26

... and the reason for the error is that the the string representation of polynomials does not correctly deal with polynomials of negative degree:

sage: str(N.gens())
'(0*a, 0*b, 0*c)'

That should be '(a, b, c)'.

Looking at a._repr_, I see that in fact Singular is to blame. And indeed, in Singular we have

> 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

So, there is a bug in Singular that should be reported upstream.

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.

[simon-king-jena]

comment:27

Singular states in its documentation that the degrees for wp need to be positive integral. So, they may not acknowledge that it is a bug. But I will try.

[simon-king-jena]

comment:28

Or it can be solved on the side of Sage. Namely, while Singular does not allow the weighted order wp(1,2,0), it does allow to provide the dp order with an additional weight vector, and this weight vector can contain negative numbers:

> 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

Couldn't we change the method TermOrder.singular_str(), such that (a(...),dp) is used if there is any non-positive weight?

[kwankyu]

comment:29

wp, that is, wdegrevlex 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.

Your term order (a(...),dp) is not a term order officially supported in Sage. In that case, Sage allows to force the term order by setting force=True So you could do

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'

But this reveals a bug in dealing with the force 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.

You are welcome to add more documentation in the reviewer patch.

[kwankyu]

fixed a bug in dealing with the force argument

[kwankyu]

comment:30

Attachment: trac_11316.4.patch.gz

I correct my mistake:

wp, that is, wdegrevlex 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.

[simon-king-jena]

comment:31

With the fourth patch, it is still allowed to do

sage: N.<a,b,c> = PolynomialRing(QQ, 3, order=TermOrder('wdeglex',[0,-2,3]))

There should an error be raised, because negative degrees do not work at all, due to the limitations imposed by Singular:

sage: a
0*a

Another example:

sage: N.<a,b,c> = PolynomialRing(QQ, 3, order=TermOrder('wdeglex',[1.1,2,3]))
sage: a.degree()
1

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:

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;'

I am not sure what to do in the second case: Raise an error, or silently convert 1.1 to 1?

[simon-king-jena]

Attachment: trac11316_reviewer.patch.gz

Raise an error on invalid degree weights. State in the docs which weights are accepted

[simon-king-jena]

Reviewer: Simon King

[simon-king-jena]

Description changed:

--- 
+++ 
@@ -1,3 +1,9 @@
 Weighted degree term orders (wp,Wp,ws,Ws as in Singular) added to TermOrder.

 New term orders as well as matrix term orders can be used in block term orders.
+
+Apply:
+
+1. [attachment: trac_11316.4.patch](https://github.com/sagemath/sage-prod/files/10652828/trac_11316.4.patch.gz)
+
+2. [attachment: trac11316_reviewer.patch](https://github.com/sagemath/sage-prod/files/10652829/trac11316_reviewer.patch.gz)

[simon-king-jena]

comment:32

I was reading the patch, and it seems ok. Meanwhile I think that __setstate__ 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 blocks does not override the new method blocks()).

Having weighted degree orders is a good thing. There are some limitations inherited from Singular: The degree weights have to be positive integers.

I give a positive review, and add a reviewer patch, which I hope is fine for you.

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 1.1 will silently be converted into 1. There are doctests for that behaviour.

Also, I add a comment in the doc of the new method blocks(), 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.

So, if you don't oppose against my reviewer patch:

Apply trac_11316.4.patch trac11316_reviewer.patch

[simon-king-jena]

comment:33

Note a related ticket: #11431, 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!

[jdemeyer]

Merged: sage-4.7.1.alpha3

[jdemeyer]

Changed merged from sage-4.7.1.alpha3 to none

[jdemeyer]

comment:35

There are some issues on Solaris. On the "marks" and "t2" buildbots, I get the following error (identical on both systems):

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)
**********************************************************************

[simon-king-jena]

comment:37

Hi Jeroen,

Replying to @jdemeyer:

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: sage: singular.eval('ring r1 = (9,x),(a,b,c,d,e,f),(M((1,2,3,0)),wp(2,3),lp)')

That test has only been introduced by #11431. Hence, I am reverting the ticket here into "positive review"

[simon-king-jena]

comment:38

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.

I have no recent working Sage version on my t2 account. Could you please test whether the line

sage: singular.eval('ring r1 = (9,x),(a,b,c,d,e,f),(M((1,2,3,0)),wp(2,3),lp)')

works without and with the patch applied?

[jdemeyer]

comment:39

Replying to @simon-king-jena:

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.

I have no recent working Sage version on my t2 account.

I have no account on t2 but I could test on mark2 instead (but this will take a while, that is a very slow machine).

[jdemeyer]

comment:40

Replying to @jdemeyer:

Replying to @simon-king-jena:

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.

I have no recent working Sage version on my t2 account.

I have no account on t2 but I could test on mark2 instead (but this will take a while, that is a very slow machine).

I am not able to build Sage on mark2 (GNUTLS fails). I don't know how the buildbot manages.

[simon-king-jena]

comment:41

Replying to @jdemeyer:

I have no account on t2 but I could test on mark2 instead (but this will take a while, that is a very slow machine).

I am not able to build Sage on mark2 (GNUTLS fails). I don't know how the buildbot manages.

Too bad. I am now trying to build on mark -- no idea whether it will work, of course.

One question: Wouldn't it be possible to use the Sage version that was built by the buildbot?

[simon-king-jena]

comment:42

As much as I understand, the ticket here is independent from #11431, right?

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.