#11316: Weighted degree term orders added
--------------------------------+-------------------------------------------
Reporter: klee | Owner: AlexGhitza
Type: enhancement | Status: needs_review
Priority: minor | Milestone: sage-4.7.1
Component: basic arithmetic | Keywords:
Work_issues: | Upstream: N/A
Reviewer: | Author: Kwankyu Lee
Merged: | Dependencies:
--------------------------------+-------------------------------------------
Comment(by SimonKing):
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.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11316#comment:18>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
