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

define forgotten ne #116

Merged
merged 1 commit into from
May 11, 2021
Merged

define forgotten ne #116

merged 1 commit into from
May 11, 2021

Conversation

sroet
Copy link
Collaborator

@sroet sroet commented May 11, 2021

while working on #115 , a test started failing with:

_______________________________ TestMutableContactTrajectory.test_hash_eq _______________________________

self = <contact_map.tests.test_contact_trajectory.TestMutableContactTrajectory object at 0x7fca5ea18dc0>

    def test_hash_eq(self):
        cmap = MutableContactTrajectory(self.traj, cutoff=0.075,
                                        n_neighbors_ignored=0)
        assert hash(cmap) != hash(self.map)
>       assert cmap != self.map
E       assert <contact_map.contact_trajectory.MutableContactTrajectory object at 0x7fca5ed678b0> != <contact_map.contact_trajectory.MutableContactTrajectory object at 0x7fca5eaa5850>
E        +  where <contact_map.contact_trajectory.MutableContactTrajectory object at 0x7fca5eaa5850> = <contact_map.tests.test_contact_trajectory.TestMutableContactTrajectory object at 0x7fca5ea18dc0>.map

contact_map/tests/test_contact_trajectory.py:222: AssertionError

Which was due to the fact the ContactTrajectory did update __eq__, but not __ne__. This fixes that oversight

Copy link
Owner

@dwhswenson dwhswenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh... I thought that __ne__ wasn't necessary in Python 3 (and we're not-intentionally-breaking-but-not-supporting Python 2). From https://docs.python.org/3/reference/datamodel.html#object.__ne__:

For __ne__(), by default it delegates to __eq__() and inverts the result unless it is NotImplemented.

Does Cython not follow that?

Either way, I have no problem with adding the extra __ne__ method. Doesn't hurt anything. Will merge when AppVeyor finishes.

@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #116 (11da8b4) into master (bcff7ce) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #116   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines         1125      1127    +2     
=========================================
+ Hits          1125      1127    +2     
Impacted Files Coverage Δ
contact_map/contact_trajectory.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcff7ce...11da8b4. Read the comment docs.

@dwhswenson dwhswenson merged commit 3089eba into dwhswenson:master May 11, 2021
@sroet
Copy link
Collaborator Author

sroet commented May 11, 2021

Does Cython not follow that?

So inheritance of compiled cython code into python is a bit special 😅
(Tested this by adding a quick print statement to the ContactObject.__eq__. )
It seems that __ne__ is inherited from ContactObject, but this is compiled in c as not ContactObject.__eq__ 😅

@sroet sroet deleted the fix_forgotten_ne branch May 11, 2021 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants