remove definitions of __ne__?

[Martin R]

If I understand correctly, in python3 it is no longer necessary to implement __ne__, if it is simply the negation of __eq__.

There are currently about 200 definitions of the form

    def __ne__(self, other):
        return not (self == other)

I think it would be good to remove them.  Travis and I are currently discussing a case where it makes sense to have a different implementation of __ne__.  If the sage codebase has only those __ne__ which are really necessary, it would make intentions clearer.

Opinions? (otherwise I would create a pull request)

Martin

[Oscar Benjamin]

On Sat, 2 Sept 2023 at 08:44, 'Martin R' via sage-devel

<sage-...@googlegroups.com> wrote:
>
> If I understand correctly, in python3 it is no longer necessary to implement __ne__, if it is simply the negation of __eq__.
>
> There are currently about 200 definitions of the form
>
> def __ne__(self, other):
> return not (self == other)
>
> I think it would be good to remove them.

SymPy removed most of its __ne__ methods but a few had to be kept. The
exception is that if any superclass defines __ne__ then a subclass
that overrides __eq__ must also override __ne__ to keep them
consistent. Obviously if you control both classes then you can remove
__ne__ from both but in SymPy's case some classes subclass builtin
types that cannot be changed:

class PolyElement(dict):
def __eq__(self, other):
# override dict.__eq__
def __ne__(self, other):
# also need to override dict.__ne__

I don't know if that applies to any of the 200 __ne__ methods in Sage
but I would check the mro in each case before removing __ne__. It is
easy for this sort of thing to be overlooked in test code and in fact
messing with __eq__/__ne__ (more so __eq__) can invalidate much of the
test suite so I would tread carefully.

--
Oscar

[Martin R]

On Saturday, 2 September 2023 at 11:39:12 UTC+2 Oscar Benjamin wrote:

On Sat, 2 Sept 2023 at 08:44, 'Martin R' via sage-devel
<sage-...@googlegroups.com> wrote:

...

 It is easy for this sort of thing to be overlooked in test code and in fact
messing with __eq__/__ne__ (more so __eq__) can invalidate much of the
test suite so I would tread carefully.

Could you provide an example?  I would think that in such a case a doctest should catch the problem - what other purpose would a doctest have?

Thank you for your input!

[Martin R]

Actually, it seems that I have already found an instance of the problem you describe, except that I do not understand it.

In sage/algebras/jordan_algebra.py we have two "def __ne__", one in JordanAlgebraSymmetricBilinear and one in SpecialJordanAlgebra.

If I remove them (moving the tests to those of __eq__), the following fails:

                sage: m = matrix([[0,1],[1,1]])
                sage: J.<a,b,c> = JordanAlgebra(m)
                sage: x = 4*a - b + 3*c
                sage: x != J((4, (-1, 3)))
                False

I have no idea why, the element class is defined directly in the parent JordanAlgebraSymmetricBilinear, and inherits only from AlgebraElement.

Apparently, the problem also arises, if _richcmp_ is implemented in a superclass.  I think I knew that once...

Martin

[Oscar Benjamin]

On Sat, 2 Sept 2023 at 12:15, 'Martin R' via sage-devel

<sage-...@googlegroups.com> wrote:
>
> On Saturday, 2 September 2023 at 11:39:12 UTC+2 Oscar Benjamin wrote:
>
> On Sat, 2 Sept 2023 at 08:44, 'Martin R' via sage-devel
> <sage-...@googlegroups.com> wrote:
>
> ...
>
> It is easy for this sort of thing to be overlooked in test code and in fact
> messing with __eq__/__ne__ (more so __eq__) can invalidate much of the
> test suite so I would tread carefully.
>
> Could you provide an example? I would think that in such a case a doctest should catch the problem - what other purpose would a doctest have?

It is not so much an issue with doctests but in SymPy most tests are
pytest-style tests that look something like:

def test_A():
assert A(1) == A(10)
assert A(2) == A(7)
assert A(3) != A(5)
...

Now if you change your class A to look like:

class A:
def __eq__(self, other):
return True
def __ne__(self, other):
return True

Then your test suite will pass regardless of what any of the rest of
the code does because every == or != returns True all the time.
Obviously you would not literally write "return True" but an
undetected bug having that sort of effect could invalidate many of the
tests so you have to be very careful how you write and test methods
like __eq__ and __ne__. That would make me nervous about wholesale
removal of __ne__ without some careful checking because it is not
*always* the case that removing __ne__ will have no effect.

The first contributor who submitted a PR for this in SymPy did not do
careful checking and did not seem to understand all of the
implications of removing __ne__ and so I did not trust the PR to be
merged regardless of the tests eventually passing (when the __ne__
methods that were still needed were restored). Subsequently the PR was
rewritten by a more experienced contributor and then merged. Some
other contributors seemed not to appreciate my concern over this at
the time but at least for SymPy removing some harmlessly redundant
__ne__ methods brought close to zero benefit in exchange for a
moderate risk of introducing bugs.

--
Oscar

[Martin R]

Thank you!

[Oscar Benjamin]

On Sat, 2 Sept 2023 at 12:43, 'Martin R' via sage-devel

<sage-...@googlegroups.com> wrote:
>
> Actually, it seems that I have already found an instance of the problem you describe, except that I do not understand it.
>
> In sage/algebras/jordan_algebra.py we have two "def __ne__", one in JordanAlgebraSymmetricBilinear and one in SpecialJordanAlgebra.
>
> If I remove them (moving the tests to those of __eq__), the following fails:
>
> sage: m = matrix([[0,1],[1,1]])
> sage: J.<a,b,c> = JordanAlgebra(m)
> sage: x = 4*a - b + 3*c
> sage: x != J((4, (-1, 3)))
> False
>
> I have no idea why, the element class is defined directly in the parent JordanAlgebraSymmetricBilinear, and inherits only from AlgebraElement.
>
> Apparently, the problem also arises, if _richcmp_ is implemented in a superclass. I think I knew that once...

Oh, that's a good point. The cases in sympy where __ne__ was needed
were from subclassing builtin types like dict. Probably that applies
to any (Python or Cython?) subclass of a Cython class that implements
__richcmp__ for exactly the same reason. Possibly that means that the
situation is more complicated in Sage than in a pure Python codebase.

There was also one other case in SymPy where __ne__ was needed
although I don't remember where. Theoretically it should have been
possible to remove the __ne__ method but it caused a test failure
under PyPy. Let me dig it up...

Oh here is the (most recent) PR. I remembered wrong and it was in fact
never merged:
https://github.com/sympy/sympy/pull/23321

The sticking point was that we never isolated the difference in
behaviour between CPython and PyPy for UndefinedFunction.__ne__ (this
will be something to do with metaclasses). PyPy might not be relevant
for Sage but I think it does at least demonstrate that removing __ne__
methods is not as trivial as you might hope and quoting myself from
the PR:

> You need to understand that removing these __ne__ methods is really not a high-priority improvement to the codebase. Literally nothing is fixed by making this change but there is the potential to break something.

Probably some contributors were a bit disheartened by this statement
but it is true: literally nothing would be fixed by removing the
__ne__ methods. We can shave 200 *probably* redundant lines from an
800000 line codebase but that's about it and there is definitely a
nonzero risk of introducing bugs.

(The reason for the disheartening is that the issue had originally
been labelled as "easy to fix" which is supposed to be an invitation
for new contributors to be coached through submitting a simple PR but
then as sometimes happens it turned out not to be as easy as expected
because it is definitely not as trivial as just deleting all the
__ne__ methods. When it turns out that investigating/reviewing a
change is much harder than editing the code and making the tests pass
the issue is no longer suitable for new contributors.)

--
Oscar