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

#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

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

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

[simon-king-jena]

comment:43

For the reason pointed out in my previous post, I think it is alright to reinstate the positive review.

[jdemeyer]

Merged: sage-4.7.2.alpha1