Skip to content

Fix C3dMapper.__eq__() early return skipping remaining keys#384

Open
yasumorishima wants to merge 5 commits intopyomeca:devfrom
yasumorishima:fix/issue-383-eq-early-return
Open

Fix C3dMapper.__eq__() early return skipping remaining keys#384
yasumorishima wants to merge 5 commits intopyomeca:devfrom
yasumorishima:fix/issue-383-eq-early-return

Conversation

@yasumorishima
Copy link
Copy Markdown

@yasumorishima yasumorishima commented Feb 12, 2026

Summary

Closes #383

Fixes two early-return bugs in C3dMapper.__eq__() that caused the comparison to stop after checking only the first key, instead of iterating through all keys.

Changes

Bug 1: C3dMapper comparison (line 58)

# Before (bug): returns after comparing only the first child
return self._storage[key] == other._storage[key]

# After (fix): only return on mismatch, continue loop otherwise
if not (self._storage[key] == other._storage[key]):
    return False

Bug 2: numpy array comparison (line 62-67)

# Before (bug): returns True after the first matching array
np.testing.assert_array_equal(self._storage[key], other._storage[key])
return True

# After (fix): only return on mismatch, using simpler np.array_equal
if not np.array_equal(self._storage[key], other._storage[key]):
    return False

Bug 3: __eq_param__ numpy array handling (line 106-108)

The fix to Bug 1 & 2 causes meta_points (a dict containing numpy arrays like residuals and camera_masks) to be properly reached and compared. The existing else branch in __eq_param__ used dict1[key] != dict2[key], which raises ValueError for numpy arrays. Fixed by adding an np.array_equal check for numpy array values.

Tests

Added test_eq() with 5 assertions:

  • Empty c3d objects are equal
  • Populated c3d objects with identical data are equal
  • Deepcopy is equal to original
  • Modifying only analogs (not points) makes them unequal — this was the core bug: previously returned True
  • Modifying only parameters (not header) makes them unequal — same bug at top level

Note on test_parse_and_rebuild failures

Some existing test_parse_and_rebuild and test_parse_and_rebuild_data tests now fail for certain formats (BTS, Optotrak, Vicon, Label2). This is not caused by this PR — it is a pre-existing issue that was masked by the broken __eq__. The old __eq__ returned True after comparing only the first key (points), so the write-read roundtrip data loss in other keys (meta_points, analogs, etc.) was never detected. The header and parameters comparisons continue to pass for all formats.


This change is Reviewable

yasumorishima and others added 5 commits February 13, 2026 02:25
Closes pyomeca#383

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The meta_points dict contains numpy arrays directly (not parameter-style
dicts), so the else branch needs to handle numpy array comparison to
avoid "truth value of an empty array is ambiguous" ValueError.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address CodeRabbit review: use np.random.default_rng(42) instead of
np.random.rand for deterministic test results.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@pariterre pariterre left a comment

Choose a reason for hiding this comment

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

:lgtm:

@pariterre reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yasumorishima).

@pariterre
Copy link
Copy Markdown
Member

@yasumorishima
Thanks for spotting and correcting this error. I reckon the tests will predictably fails, so no bother! I am currently in vacation, so I'll accept the PR as is, and fix the tests when I come back to work :)

@yasumorishima
Copy link
Copy Markdown
Author

Thank you for the review and approval! No rush at all — enjoy your vacation. I'm happy to help with anything when you're back.

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.

Bug: C3dMapper.__eq__() returns early, skipping comparison of remaining keys

2 participants