39 views

Skip to first unread message

Sep 2, 2023, 3:44:46 AMSep 2

to sage-devel

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)

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

Sep 2, 2023, 5:39:12 AMSep 2

to sage-...@googlegroups.com

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

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

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

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

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

Sep 2, 2023, 7:15:04 AMSep 2

to sage-devel

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!

Sep 2, 2023, 7:43:30 AMSep 2

to sage-devel

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

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

Sep 2, 2023, 7:44:46 AMSep 2

to sage-...@googlegroups.com

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

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

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

> 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?

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

Sep 2, 2023, 8:20:36 AMSep 2

to sage-devel

Thank you!

Sep 2, 2023, 10:59:45 AMSep 2

to sage-...@googlegroups.com

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

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

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

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

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

Reply all

Reply to author

Forward

0 new messages

Search

Clear search

Close search

Google apps

Main menu