Skip to content

[Python] Remove TObject __eq__ pythonization #19145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guitargeek
Copy link
Contributor

The TObject __eq__ pythonization calls TObject::Equals(), which by default does a pointer comparison. That can be very confusing for Python users.

The TObject `__eq__` pythonization calls `TObject::Equals()`, which by
default does a pointer comparison. That can be very confusing for Python
users.
@vepadulano
Copy link
Member

Just a fly-by thought, this pythonization has been around for so many years that, while I still strongly believe its current implementation is buggy, I am afraid this will introduce a backwards-incompatible behaviour far, far worse than #19061. One possible way to mitigate this is to have a check that if there is no operator == implementation (or equivalent) for a class, then we can raise an informative exception. Actually, we should probably just raise a warning and declare a deprecation phase for this. Also, note that while the discussion at #19061 is regarding TH1 classes and derived only, this PR is going to have effects for every TObject derived class.

Copy link

Test Results

    20 files      20 suites   3d 19h 51m 59s ⏱️
 3 043 tests  3 043 ✅ 0 💤 0 ❌
59 159 runs  59 159 ✅ 0 💤 0 ❌

Results for commit 37f4365.

@guitargeek
Copy link
Contributor Author

guitargeek commented Jun 24, 2025

That's exactly my intention! I wanted to see what the CI says, and if there are no problems deprecate this Pythonization.

I am afraid this will introduce a backwards-incompatible behaviour far, far worse than #19061.

In my opinion, the silent behavior changes that are far worse. Removing a method like suggested by this PR at least gives you an error (or warning after this PR is updated).

You also have to see the picture with ROOT 6.34 before the UHI introduction in 6.36. What you guys did with the new __eq__ was to introduce a change of behavior without documenting it, and as you can see from our tests and #19035 this even confused our own code. So it definitely has the potential to create bugs in user code. Doesn't matter how wrong or "buggy" you think the old behavior of comparing by pointer was: it has been there for many years as you said and people might rely on it.

Also, note that while the discussion at #19061 is regarding TH1 classes and derived only, this PR is going to have effects for every TObject derived class.

Yes, and this is a bonus 🙂

@vepadulano
Copy link
Member

Yes, and this is a bonus 🙂

Only if we make sure this won't disrupt users' code. As things stand, this PR cannot be merged. My suggestion is to start a deprecation phase with a warning, then make it an exception or not implemented in a later ROOT release like 6.40 or 6.42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants