MathGlyph may need __lt__ and __eq__ for Python 3 support
Closed this issue · 12 comments
Right now the mutatorMath InstanceWriter cannot properly generate glyphs on Python 3 because MathGlyph is missing the required __lt__ and __eq__ methods (see also LettError/MutatorMath#65 for a similar issue). Here we have the current __cmp__:
I patched my local copy of mathGlyph.py with a simple:
def __eq__(self, other):
self.__cmp__(other)
def __lt__(self, other):
self.__cmp__(other)This all works fine, and mutatorMath is happy. I wonder what the right behavior for a “proper” __lt__ would be, though, if any.
Good catch!
__cmp__ does not work any more on Python 3.
We could implement all the rich comparison operators or only implement some and use the functools.total_ordering class decorator to fill in the rest. However, by looking at MathGlyph.__cmp__ it appears to be used only to check if two glyphs are the same or not.
It just returns True (1) or False (0), so it's really only meant to do == and !=, and not to do stuff like sorting where you need to return all -1, 0 and 1.
So we can just replace it with __eq__ and __ne__ and be done with it.
The __lt__ etc. stuff never worked anyway.
Or would you like to also "sort" MathGlyphs? Would that be a reasonable thing to do? Hm, maybe not.
Sorry folks, but MutatorMath actually does sort glyphs so either we change its code or we need __lt__ in MathGlyph as per the original bug.
hmm, actually you're right.
This is python 2.7.13:
>>> from fontMath.mathGlyph import MathGlyph
>>> m1 = MathGlyph(None)
>>> m2 = MathGlyph(None)
>>> sorted([m1, m2])
[<fontMath.mathGlyph.MathGlyph object at 0x1004e2b10>, <fontMath.mathGlyph.MathGlyph object at 0x1004e2b90>]But on python 3.6.0 this raises TypeError, and rightly so as we don't have __lt__:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: '<' not supported between instances of 'MathGlyph' and 'MathGlyph'But given the way equality is defined, how do we define whether a MathGlyph is "greater than" another?
What does that actually mean?
And where/why is MutatorMath sorting MathGlyphs?
it's probably in here:
https://github.com/LettError/MutatorMath/blob/ef424ab2cad0d40b9476accaa2c5dc17f9749e8a/Lib/mutatorMath/objects/mutator.py#L29-L30
The comment however says:
the order itself does not matter, but we should always build in the same order.
The problem with rich comparison operators is that once you override one, you have to override them all.
So I guess, in order to keep the current (partly broken) functionality of __cmp__, we'll have to support at least __eq__, __ne__ and __lt__.
I still don't know what __lt__ means for a MathGlyph though. Suggestions are welcome.
ok, not all of them, but at least __eq__ and __lt__, like MathInfo and MathKerning do by the way.
I noticed that MathInfo and MathKerning define __eq__ without also defining __ne__. This is not recommend according to python 2 docs:
There are no implied relationships among the comparison operators. The truth of x==y does not imply that x!=y is false. Accordingly, when defining eq(), one should also define ne() so that the operators will behave as expected.
Whereas on Python 3 __ne__ already does the right thing:
!=returns the opposite of==, unless==returnsNotImplemented
https://docs.python.org/3.0/whatsnew/3.0.html#operators-and-special-methods
So if we want to support both python 2 and 3, we have to define a __ne__ every time we define a __eq__.
There was a problem with tiny rounding errors that seemed to come out of no where. By sorting the locations the order in which the math was done became predictable. Maybe sorting the glyphs was overkill, I can't actually test it right now and I don't want to take it out without testing.
The TypeError: '<' not supported between instances of 'MathGlyph' and 'MathGlyph' occurs at this line inside Mutator.getFactors method:
it only occurs when, in the deltas list of tuples, the first item in the tuples (factor) is the same in all of them, thus python proceeds to compare the second items, mathItem, which is an instance of MathGlyph and does not have an __lt__ method.
There may be other places where MutatorMath attempts to sort MathGlyph objects, but in my tests this line triggers the issue more frequently.
Now, if I simply don't sort that deltas list, the process completes without exceptions also under python3. And when I compare the output with/without sorted I don't get any differences.
deltas is the value returned by getFactors; this seems to be only used in another method, getInstance.
Each math item is scaled by its factor and the total sum of the scaled math items is accumulated.
Now, given the commutative property of addition, changing the order of the operands does not change the result. So, perhaps, the sorted(deltas) is unnecessary?
We could add a __lt__ method to fontMath MathGlyph, however we have to decide how to implement this.
MathGlyphs are not scalar values that can be easily compared, but they are complex structures with many nested, optional members.
Should we sort MathGlyphs by comparing each non-None attribute of self and other (provided they in turn implement __lt__)? If so, in which order?
We could add a lt method to fontMath MathGlyph, however we have to decide how to implement this.
MathGlyphs are not scalar values that can be easily compared, but they are complex structures with many nested, optional members.
I haven't had time to look into this beyond skimming a few lines of code in fontMath and MutatorMath. But, I'm not really comfortable implementing __lt__ in MathGlyph for the reasons that you stated. It's going to be an arbitrary decision that will always be right and wrong. I can be convinced otherwise if a change in MutatorMath isn't possible.
Theoretically the order of operands does not matter, but I suspected that with adding many small floats there were differences and I wanted to exclude it as a possible source.
Rather than add a __lt__ method to MathGlyph, I think it might be enough to change line https://github.com/LettError/MutatorMath/blob/0330ef3af5d928e01bdef68fe26d5910ea4ebcfa/Lib/mutatorMath/objects/mutator.py#L240 to
deltas = sorted(deltas, key=itemgetter(0), reverse=True)
That would no longer expose MathGlyph to the unwanted pressure of having to form an opinion of another glyph.
@LettError the fix you propose works for me on Python 3(.6.0), with no change needed in fontMath and the whole mutatorMath test suite passing!
This commit LettError/MutatorMath@c0c462e works around the issue by avoiding to sort MathGlyph instances.
Since fontMath doesn't need to be modified, we can close it.
BTW, I tagged a new MutatorMath 2.0.2 which includes that fix.
It's the first version of MutatorMath that actually works on Python 3.
Special thanks to @verbosus and @LettError! 🎉