Make skeleton nodes mutable #135
Conversation
WalkthroughThe changes involve significant modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
tests/model/test_skeleton.py (2)
78-89: LGTM! Consider adding docstring for clarity.The test comprehensively verifies the node flipping behavior with and without symmetries. The assertions properly check both the flipped indices and symmetry object properties.
Add a docstring to explain the test's purpose:
def test_get_flipped_node_inds(): + """Test node index flipping behavior with and without symmetries. + + Verifies that: + 1. Without symmetries, indices remain unchanged + 2. With symmetries, paired nodes swap indices + 3. Symmetry objects maintain correct node relationships + """ skel = Skeleton(["A", "BL", "BR", "C", "DL", "DR"])
Line range hint
1-1: Add dedicated test for node hash stability.Given that a key objective of this PR is to maintain node hashability while allowing mutation, consider adding a dedicated test function to verify this behavior.
Add this test function:
def test_node_hash_stability(): """Verify that Node objects maintain stable hash values when mutated.""" node = Node("test") original_hash = hash(node) # Verify hash remains stable through various mutations node.x = 10 assert hash(node) == original_hash node.name = "new_name" assert hash(node) == original_hash # Verify nodes with same attributes are distinct other_node = Node("test") assert hash(other_node) != original_hash assert node != other_nodesleap_io/model/skeleton.py (2)
188-208: Fix docstring example to use method name instead of property.The docstring example incorrectly references
skel.flipped_node_indsbut should useskel.get_flipped_node_inds()to match the actual method name.Apply this change to the docstring:
- >>> skel.flipped_node_inds + >>> skel.get_flipped_node_inds()
253-255: Use isinstance() for type checking.Replace direct type comparisons with
isinstance()for more robust type checking.Apply this change:
- node_name = node.name if type(node) == Node else node + node_name = node.name if isinstance(node, Node) else node🧰 Tools
🪛 Ruff
253-253: Use
isandis notfor type comparisons, orisinstance()for isinstance checks(E721)
tests/io/test_slp.py (1)
Line range hint
1-1: Add test coverage for node mutability behaviorThe test file lacks coverage for key behaviors described in the PR objectives:
- Verifying that
Node("a") != Node("a")- Confirming that mutating a Node's attributes doesn't change its hash value
- Testing node attribute access and modification
Consider adding a new test function to verify these behaviors:
def test_node_mutability(): """Test Node mutability and identity behavior.""" # Test that nodes with same attributes are different objects assert Node("a") != Node("a") # Test that mutating attributes doesn't change identity node = Node("a") node_hash = hash(node) node.name = "b" assert hash(node) == node_hash # Test attribute access and modification node = Node("test") assert node.name == "test" node.name = "modified" assert node.name == "modified"Would you like me to help implement these test cases or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
sleap_io/model/skeleton.py(4 hunks)tests/io/test_slp.py(3 hunks)tests/model/test_labels.py(1 hunks)tests/model/test_skeleton.py(3 hunks)
🧰 Additional context used
🪛 Ruff
sleap_io/model/skeleton.py
253-253: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
256-256: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
🔇 Additional comments (9)
sleap_io/model/skeleton.py (4)
14-14: LGTM: Node mutability change aligns with requirements.
The decorator change from @define(frozen=True, cache_hash=True) to @define(eq=False) correctly implements the desired behavior where nodes are mutable and compared by identity rather than value, ensuring Node("a") != Node("a") while allowing attribute modifications.
174-186: LGTM: Well-designed symmetry properties.
The new properties provide a clean interface with:
- Consistent ordering through sorted indices
- Proper type hints
- Clear mapping between indices and names
307-310: LGTM: Clean symmetry handling refactor.
The use of a local symmetry variable improves code readability while maintaining proper node addition behavior.
14-14: Verify node mutability and identity behavior.
Let's confirm that the changes achieve the desired node behavior.
✅ Verification successful
Node mutability and identity behavior confirmed
The verification shows that the changes achieve the desired behavior:
- Node class is defined with
@define(eq=False), ensuring identity-based comparison - Nodes are used as dictionary keys in
_node_ind_mapwhich relies on identity-based comparison - Node instances can be mutated as evidenced by:
- No
frozen=Truedecorator on the Node class - Direct modification of nodes in
_convert_nodesmethod - Mutable list of nodes with
on_setattrcallback
- No
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that Node instances are used as dictionary keys based on identity
# rather than value, and that they can be mutated.
# Search for dictionary usage with Node keys
rg -l "Dict\[.*Node.*\]|dict.*Node"
# Search for Node mutation after being used as a key
rg -A 5 "node\.[a-zA-Z_]+ = "
Length of output: 123
Script:
#!/bin/bash
# Let's examine the Node class implementation and its usage more thoroughly
# First, check the Node class definition and its attributes
rg -A 10 "class Node" sleap_io/model/skeleton.py
# Check how Node instances are used in dictionaries
rg -A 5 "Dict\[.*Node" sleap_io/model/skeleton.py sleap_io/model/instance.py
# Look for any dictionary operations with Node keys
rg "dict\(.*node.*\)|{.*node.*}" sleap_io/model/skeleton.py sleap_io/model/instance.py
# Check for any mutation patterns
rg -A 3 "def.*node" sleap_io/model/skeleton.py sleap_io/model/instance.py
Length of output: 2836
Script:
#!/bin/bash
# Let's examine the Node class implementation and its usage in more detail
# Check the complete Node class implementation including any decorators
rg -B 5 -A 15 "@define" sleap_io/model/skeleton.py
# Check for dictionary usage with escaped braces
rg -A 3 "\{.*?node.*?\}" sleap_io/model/skeleton.py
# Look for Node instances in dictionary comprehensions
rg -A 3 "= \{.*?node.*?for" sleap_io/model/skeleton.py
# Check for any instance attribute modifications
rg -A 3 "setattr|__setattr__|\.name\s*=" sleap_io/model/skeleton.py
Length of output: 4261
tests/io/test_slp.py (3)
95-96: LGTM: Robust symmetry verification using node names
The change from comparing Node objects to checking symmetry_names is a good approach that aligns with making nodes mutable while maintaining reliable symmetry verification.
160-163: LGTM: Proper encapsulation of flipped node indices
The change to use get_flipped_node_inds() method instead of directly accessing the property improves encapsulation while maintaining the verification of serialization/deserialization correctness.
230-231: LGTM: Consistent use of new API across multiple skeletons
The test properly verifies flipped node indices for multiple skeletons using the new get_flipped_node_inds() method, maintaining consistency with the updated API.
tests/model/test_labels.py (2)
23-23: LGTM: Good practice to reuse the Skeleton instance.
Creating a single skel instance and reusing it across multiple test cases is a good practice. This approach:
- Reduces redundancy in the test code
- Ensures consistent testing conditions
- Aligns with the new mutable Node design where identity matters more than value equality
30-31: Verify instance creation with mutable nodes.
The test creates both regular and predicted instances using the same skeleton. However, we should verify that the instances maintain correct behavior with mutable nodes.
Let's check if there are any other tests specifically covering node mutability:
Consider adding explicit assertions to verify:
- Instances remain valid when their skeleton's nodes are mutated
- Different instances using the same skeleton maintain independence
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Node mutability is handled safely by design
The Instance class maintains points in a dictionary with Node objects as keys, and the test suite comprehensively covers Instance initialization and comparison scenarios. The skeleton's nodes are used as reference keys rather than being directly modified, which inherently maintains instance independence when sharing skeletons.
- The test at
tests/model/test_instance.pyverifies instance equality semantics and proper point-to-node mapping - Instance comparison tests explicitly verify that instances with the same skeleton maintain their independence
- The Instance class implementation in
sleap_io/model/instance.pyshows points are stored in a separate dictionary, isolating them from skeleton mutations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests related to node mutability
rg -l "Node.*mutable|mutable.*Node" "tests/"
# Search for node-related test assertions
ast-grep --pattern 'assert$_($_, Node($_))'
Length of output: 90
Script:
#!/bin/bash
# Let's search for test files related to Instance and Node classes
rg -l "class.*Instance|Instance.*test" tests/
# Look for test cases involving Instance and skeleton/nodes
rg -A 5 "def test.*skeleton|test.*node" tests/
# Search for Instance initialization patterns
ast-grep --pattern 'Instance($$$)'
# Search for test assertions involving Instance objects
ast-grep --pattern 'assert$_($_, Instance($_))'
Length of output: 5790
Script:
#!/bin/bash
# Let's look at Instance test cases to see if node mutability is covered
rg -A 10 "test.*instance" tests/model/test_instance.py
# Check for any mutation-related tests in Instance class
rg -A 5 "def test.*mut|test.*change" tests/model/test_instance.py
# Look at the Instance class implementation to understand node handling
rg -A 10 "class Instance" sleap_io/model/
Length of output: 2596
| with pytest.raises(ValueError): | ||
| skel.add_node("B") | ||
| assert skel.node_names == ["A", "B", "C"] |
There was a problem hiding this comment.
Remove unreachable assertion in error handling test.
The assertion after raise ValueError is unreachable. The test should be restructured to properly verify the error case.
Apply this change:
with pytest.raises(ValueError):
skel.add_node("B")
- assert skel.node_names == ["A", "B", "C"]
+ # Verify state remains unchanged after failed operation
+ assert skel.node_names == ["A", "B", "C"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with pytest.raises(ValueError): | |
| skel.add_node("B") | |
| assert skel.node_names == ["A", "B", "C"] | |
| with pytest.raises(ValueError): | |
| skel.add_node("B") | |
| # Verify state remains unchanged after failed operation | |
| assert skel.node_names == ["A", "B", "C"] |
| assert skel.symmetry_inds == [(0, 1)] | ||
| assert skel.symmetry_names == [("A", "B")] | ||
|
|
||
| # Don't duplicate reversed symmetries | ||
| skel.add_symmetry("B", "A") | ||
| assert skel.symmetries == [Symmetry([Node("A"), Node("B")])] | ||
| assert skel.symmetry_inds == [(0, 1)] | ||
| assert skel.symmetry_names == [("A", "B")] | ||
|
|
||
| # Add new symmetry with new node objects | ||
| skel.add_symmetry(Symmetry([Node("C"), Node("D")])) | ||
| assert skel.symmetries == [ | ||
| Symmetry([Node("A"), Node("B")]), | ||
| Symmetry([Node("C"), Node("D")]), | ||
| ] | ||
| assert skel.symmetry_inds == [(0, 1), (2, 3)] | ||
|
|
||
| # Add new symmetry with node names | ||
| skel.add_symmetry("E", "F") | ||
| assert skel.symmetries == [ | ||
| Symmetry([Node("A"), Node("B")]), | ||
| Symmetry([Node("C"), Node("D")]), | ||
| Symmetry([Node("E"), Node("F")]), | ||
| ] | ||
| assert skel.symmetry_inds == [(0, 1), (2, 3), (4, 5)] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding mutability verification.
The test thoroughly covers symmetry addition scenarios, but given the PR's focus on node mutability, consider adding a test case that verifies nodes remain mutable after being added to symmetries.
Add this test case:
# Add new symmetry with node names
skel.add_symmetry("E", "F")
assert skel.symmetry_inds == [(0, 1), (2, 3), (4, 5)]
+
+ # Verify nodes remain mutable after symmetry addition
+ node = skel["E"]
+ original_hash = hash(node)
+ node.x = 42
+ assert node.x == 42
+ assert hash(node) == original_hash # Hash should be stable📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert skel.symmetry_inds == [(0, 1)] | |
| assert skel.symmetry_names == [("A", "B")] | |
| # Don't duplicate reversed symmetries | |
| skel.add_symmetry("B", "A") | |
| assert skel.symmetries == [Symmetry([Node("A"), Node("B")])] | |
| assert skel.symmetry_inds == [(0, 1)] | |
| assert skel.symmetry_names == [("A", "B")] | |
| # Add new symmetry with new node objects | |
| skel.add_symmetry(Symmetry([Node("C"), Node("D")])) | |
| assert skel.symmetries == [ | |
| Symmetry([Node("A"), Node("B")]), | |
| Symmetry([Node("C"), Node("D")]), | |
| ] | |
| assert skel.symmetry_inds == [(0, 1), (2, 3)] | |
| # Add new symmetry with node names | |
| skel.add_symmetry("E", "F") | |
| assert skel.symmetries == [ | |
| Symmetry([Node("A"), Node("B")]), | |
| Symmetry([Node("C"), Node("D")]), | |
| Symmetry([Node("E"), Node("F")]), | |
| ] | |
| assert skel.symmetry_inds == [(0, 1), (2, 3), (4, 5)] | |
| assert skel.symmetry_inds == [(0, 1)] | |
| assert skel.symmetry_names == [("A", "B")] | |
| # Don't duplicate reversed symmetries | |
| skel.add_symmetry("B", "A") | |
| assert skel.symmetry_inds == [(0, 1)] | |
| assert skel.symmetry_names == [("A", "B")] | |
| # Add new symmetry with new node objects | |
| skel.add_symmetry(Symmetry([Node("C"), Node("D")])) | |
| assert skel.symmetry_inds == [(0, 1), (2, 3)] | |
| # Add new symmetry with node names | |
| skel.add_symmetry("E", "F") | |
| assert skel.symmetry_inds == [(0, 1), (2, 3), (4, 5)] | |
| # Verify nodes remain mutable after symmetry addition | |
| node = skel["E"] | |
| original_hash = hash(node) | |
| node.x = 42 | |
| assert node.x == 42 | |
| assert hash(node) == original_hash # Hash should be stable |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #135 +/- ##
==========================================
- Coverage 96.50% 96.13% -0.37%
==========================================
Files 17 17
Lines 2115 2124 +9
==========================================
+ Hits 2041 2042 +1
- Misses 74 82 +8 ☔ View full report in Codecov by Sentry. |
This PR introduces an overhaul to how we define equality for skeleton objects to enable skeleton mutability.
Previously, we tested for equality between instances of classes like
SkeletonandNodeby value. This was convenient so that we could doNode("a") == Node("a")and getTrue.We didn't use that pattern often, but we do use
Nodeobjects as keys (e.g., inInstanceobjects), meaning that they have to be hashable. If two objects are equal, then they must have the same hash. This also means that they have to be immutable (see this explanation).This PR makes
Nodeobjects mutable at the cost of making them hashable by ID, e.g.:Summary by CodeRabbit
New Features
Bug Fixes
Tests
Skeletonclass to improve clarity and robustness, focusing on symmetry and edge management.