Skip to content
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

Fix __eq__ and __ne__ for classes implementing them #707

Merged
merged 1 commit into from
Sep 22, 2024
Merged

Conversation

alexer
Copy link
Contributor

@alexer alexer commented Sep 22, 2024

Main issue, which concerns Vector, Location, and ShapeList:

Comparison with an object of a different type should not cause an exception - they are simply not equal. Raising an exception in __eq__ can (and will*) break unrelated code that expects __eq__ to be well-behaved.

(* I noticed this bug when cq-editor choked on it while trying to find a name for an object in a dictionary of local variables)

There's a second more minor issue, which concerns the rest of the classes:

When the other type in __eq__ is not supported, one should technically return NotImplemented instead of False, to allow the other type to take part in the comparison, in case they know about our type.

(__ne__ should also not generally be implemented as just the negation of __eq__ because of this, but the one for Plane can just be removed - Python will automatically do the right thing based on __eq__ there -- ShapeList, on the other hand, needs to also have its __ne__ overridden, because it directly subclasses list, which already implements it)

Technically, the __eq__ for Vector and Plane is also broken in another way: It's not transitive.

>>> a, b, c = Vector(0), Vector(9e-6), Vector(18e-6)
>>> a == b == c
True
>>> a == c
False

They should really eg. have a separate is_close() or something for approximate comparison, but this isn't fixed here, since I have no idea how many places it'd break, for one.

I also noticed that Shape.__eq__ calls Shape.is_same instead of Shape.is_equal which just seems... wrong.

Main issue, which concerns Vector, Location, and ShapeList:

Comparison with an object of a different type should not cause an
exception - they are simply not equal. Raising an exception in __eq__
can (and will*) break unrelated code that expects __eq__ to be well-behaved.

(* I noticed this bug when cq-editor choked on it while trying to find
   a name for an object in a dictionary of local variables)

There's a second more minor issue, which concerns the rest of the classes:

When the other type in __eq__ is not supported, one should technically
return NotImplemented instead of False, to allow the other type to take
part in the comparison, in case they know about our type.

(__ne__ should also not generally be implemented as just the negation of
__eq__ because of this, but that's also a moot point because the __ne__
can just be removed - Python will automatically do the right thing based
on __eq__ here)

Technically, the __eq__ for Vector and Plane is also broken in another way:
It's not transitive.

>>> a, b, c = Vector(0), Vector(9e-6), Vector(18e-6)
>>> a == b == c
True
>>> a == c
False

They should really eg. have a separate is_close() for approximate comparison,
but this isn't fixed here, since I have no idea how many places it'd break,
for one.
@gumyr gumyr merged commit 720bee9 into gumyr:dev Sep 22, 2024
11 checks passed
@gumyr
Copy link
Owner

gumyr commented Sep 22, 2024

Thanks! A lot of effort has been put into making the code fully align with Python (the original CadQuery code which this is based off didn't follow Python conventions) but this was area that needed work.

You are correct that Shape.__eq__ isn't really doing what an == operator should do but there is no way to compare CAD models to create the == operator otherwise. Too Tall Toby uses a volume (actually weight) comparison but that can easily be shown to be incomplete.

With respect to the transitive property, I don't know of a way to solve this problem given some tolerance must be provided. The Python isclose function works with tolerance values too. If you know of a robust solution to this please let me know.

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