On Sat, 2 Sept 2023 at 12:43, 'Martin R' via sage-devel <sage-devel@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 -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/CAHVvXxRHo66ugbSivBem0%2BVZSOZEG8s3Mr3JHgKXF%3DMi1K27Pg%40mail.gmail.com.
