Opened 9 years ago
Closed 8 years ago
#11316 closed enhancement (fixed)
Weighted degree term orders added
Reported by: | klee | Owned by: | AlexGhitza |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.2 |
Component: | basic arithmetic | Keywords: | |
Cc: | burcin, mpatel | Merged in: | sage-4.7.2.alpha1 |
Authors: | Kwankyu Lee | Reviewers: | Simon King |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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:
Attachments (5)
Change History (50)
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
- Status changed from needs_review to needs_work
comment:3 Changed 9 years ago by
- Status changed from needs_work to needs_review
Regarding to converting "blocks" to "blocks()", Jason remarks:
To my understanding, we prefer methods to properties because:
- Methods have docstrings and can be introspected (using question mark)
to obtain the docs.
- 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...
comment:4 Changed 9 years ago by
- Status changed from needs_review to needs_work
- 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.
comment:5 follow-up: ↓ 6 Changed 8 years ago by
- Status changed from needs_work to needs_review
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
http://trac.sagemath.org/sage_trac/ticket/10768
I change the status of this ticket to "needs review" to get attention. Is "needs info" the right status?
comment:6 in reply to: ↑ 5 Changed 8 years ago by
- Status changed from needs_review to needs_work
Replying to klee:
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
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.
Changed 8 years ago by
comment:7 Changed 8 years ago by
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.
comment:8 Changed 8 years ago by
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.
comment:9 Changed 8 years ago by
- Cc burcin added
comment:10 Changed 8 years ago by
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)
comment:11 follow-up: ↓ 14 Changed 8 years ago by
- Status changed from needs_work to needs_review
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.
comment:12 Changed 8 years ago by
comment:13 Changed 8 years ago by
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.
comment:14 in reply to: ↑ 11 Changed 8 years ago by
Replying to klee:
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.
comment:15 Changed 8 years ago by
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:
- Introduce a
__getattr__
method, that returns the default weights and puts them intoS.__dict__['_weights']
. That ensures thatS.__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.
- 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 doT._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?
comment:16 Changed 8 years ago by
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.
comment:17 follow-up: ↓ 18 Changed 8 years ago by
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.
Changed 8 years ago by
rebased to Sage 4.7; replaced double underscored attributes to single underscored ones
comment:18 in reply to: ↑ 17 Changed 8 years ago by
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:
- 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.
- Breaking the pickle jar for the sake of the trivial addition of an attribute is a no-go.
- As much as I see, absolutely any unpickling problem can be solved without to change the main code body.
- Here, the unpickling problem could be solved by replacing your
__setstate__
method with a single line of code.
- 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.
comment:19 Changed 8 years ago by
- Priority changed from minor to major
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.
comment:20 Changed 8 years ago by
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.
comment:21 Changed 8 years ago by
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.
comment:22 Changed 8 years ago by
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.
comment:23 follow-up: ↓ 24 Changed 8 years ago by
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.
comment:24 in reply to: ↑ 23 Changed 8 years ago by
Hi Kwankyu,
Replying to klee:
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 oldTermOrder(
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
comment:25 Changed 8 years ago by
- Status changed from needs_review to needs_work
- Work issues set to Singular conversion with negative degree weights
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".
comment:26 Changed 8 years ago by
- Status changed from needs_work to needs_review
- Work issues Singular conversion with negative degree weights deleted
... 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.
comment:27 Changed 8 years ago by
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.
comment:28 Changed 8 years ago by
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?
comment:29 Changed 8 years ago by
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.
comment:30 Changed 8 years ago by
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.
comment:31 Changed 8 years ago by
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
?
Changed 8 years ago by
Raise an error on invalid degree weights. State in the docs which weights are accepted
comment:32 Changed 8 years ago by
- Description modified (diff)
- Reviewers set to Simon King
- Status changed from needs_review to positive_review
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
comment:33 Changed 8 years ago by
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!
comment:34 Changed 8 years ago by
- Merged in set to sage-4.7.1.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
comment:35 follow-up: ↓ 37 Changed 8 years ago by
- Merged in sage-4.7.1.alpha3 deleted
- Resolution fixed deleted
- Status changed from closed to new
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) **********************************************************************
comment:36 Changed 8 years ago by
- Status changed from new to needs_review
comment:37 in reply to: ↑ 35 Changed 8 years ago by
- Status changed from needs_review to positive_review
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"
comment:38 follow-up: ↓ 39 Changed 8 years ago by
- Status changed from positive_review to needs_info
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?
comment:39 in reply to: ↑ 38 ; follow-up: ↓ 40 Changed 8 years ago by
Replying to SimonKing:
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).
comment:40 in reply to: ↑ 39 ; follow-up: ↓ 41 Changed 8 years ago by
- Cc mpatel added
Replying to jdemeyer:
Replying to SimonKing:
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 onmark2
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.
comment:41 in reply to: ↑ 40 Changed 8 years ago by
Replying to jdemeyer:
I have no account on
t2
but I could test onmark2
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?
comment:42 Changed 8 years ago by
- Status changed from needs_info to needs_review
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.
comment:43 Changed 8 years ago by
- Status changed from needs_review to positive_review
For the reason pointed out in my previous post, I think it is alright to reinstate the positive review.
comment:44 Changed 8 years ago by
- Milestone changed from sage-4.7.1 to sage-4.7.2
comment:45 Changed 8 years ago by
- Merged in set to sage-4.7.2.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
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.