Handle skeleton encoding internally #1970
Conversation
WalkthroughA new class named Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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}
sleap/skeleton.py
Outdated
| class SkeletonEncoder: | ||
| """Replace json.pickle.encode with our own encoder.""" |
There was a problem hiding this comment.
Add the missing encoder implementation.
The SkeletonEncoder class is currently empty. To fulfill its purpose of replacing json.pickle.encode with a custom encoder, please add the necessary encoding logic within the class definition.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1970 +/- ##
===========================================
+ Coverage 73.30% 75.50% +2.19%
===========================================
Files 134 133 -1
Lines 24087 24625 +538
===========================================
+ Hits 17658 18593 +935
+ Misses 6429 6032 -397 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
sleap/skeleton.py (3)
88-117: LGTM! Remember to implement the_encodemethod.The
SkeletonEncoderclass is well-documented and provides a clear explanation of its purpose and the encoding format. Theencodeclass method,_encode_node, and_encode_edge_typemethods are implemented correctly.However, the
_encodemethod, which is intended for the actual encoding logic, is currently unimplemented. Please ensure that this method is implemented to complete the functionality of theSkeletonEncoderclass.Do you want me to generate the implementation for the
_encodemethod or open a GitHub issue to track this task?
119-121: LGTM! Consider adding a comment to clarify the purpose of_encoded_strings.The
__init__method is correctly defined and initializes the_encoded_stringsinstance variable. However, the purpose of this variable is not clear from the provided context.Consider adding a comment to explain how
_encoded_stringsis intended to be used within theSkeletonEncoderclass.
Line range hint
1090-1115: LGTM! Remember to remove or comment out debug print statements before production.The added debug print statements in the
to_jsonmethod do not alter the existing functionality and can be useful for logging and debugging purposes during development.However, these print statements may clutter the output and should be removed or commented out before deploying the code to production to avoid unnecessary logging.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/skeleton.py (3 hunks)
Additional context used
Ruff
sleap/skeleton.py
138-138: SyntaxError: Expected an indented block after function definition
Additional comments not posted (4)
sleap/skeleton.py (4)
122-133: LGTM! Remember to implement the_encodemethod.The
encodeclass method is correctly defined and documented. It creates an instance of theSkeletonEncoderclass and calls the_encodemethod on the instance, which is the expected behavior.However, since the
_encodemethod is currently unimplemented, calling theencodemethod will raise an error. Please ensure that the_encodemethod is implemented to enable the functionality of theencodemethod.
135-137: Reminder: Implement the_encodemethod.The
_encodemethod is correctly defined with the appropriate parameters and return type. However, the method is currently unimplemented, as indicated by the empty body.Please ensure that the
_encodemethod is implemented to enable the functionality of theSkeletonEncoderclass, as it is responsible for the actual encoding logic.
138-151: LGTM!The
_encode_nodemethod is correctly defined and documented. It takes aNodeobject as input and returns a dictionary representation of the node, following the expected format.The dictionary representation includes the
py/objectkey set tosleap.Skeleton.Nodeand thepy/statekey containing a tuple of the node's name and weight, which aligns with the provided encoding format.Tools
Ruff
138-138: SyntaxError: Expected an indented block after function definition
153-168: LGTM!The
_encode_edge_typemethod is correctly defined and documented. It takes anEdgeTypeobject as input and returns a dictionary representation of the edge type, following the expected format.The dictionary representation includes the
py/reducekey, which contains a list of dictionaries representing the type and value of the edge type. This aligns with the provided encoding format forEdgeTypeobjects.
…lways the same locally
…hat we do not always get py/id
sleap/skeleton.py
Outdated
|
|
||
| json_str = jsonpickle.encode(data) | ||
| # json_str = jsonpickle.encode(data) | ||
| encoder = SkeletonEncoder() |
There was a problem hiding this comment.
SkeletonEncoder.encode(data) instead
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (5)
environment_no_cuda.yml (1)
Line range hint
1-47: Consider reviewing version constraints for consistencyWhile the change to the
attrspackage version constraint is good, it might be beneficial to review the version constraints of other dependencies in this file for consistency. Some packages have specific version constraints, while others have more relaxed ones. Consider standardizing the approach to version constraints across all dependencies to ensure optimal compatibility and reproducibility.tests/test_skeleton.py (2)
12-36: LGTM: Comprehensive test for Skeleton encoding and decoding.This test function thoroughly checks the encoding and decoding process for a Skeleton object loaded from a JSON file. It verifies both object equality and JSON string equality, which is excellent.
A minor suggestion for improvement:
Consider adding an assertion to check that the
encoded_json_stris not empty before parsing it. This could help catch potential encoding failures more explicitly:assert encoded_json_str, "Encoded JSON string should not be empty"This assertion could be added just after line 23.
39-67: LGTM: Well-structured parameterized test for multiple Skeleton fixtures.This parameterized test function provides excellent coverage by testing the encoding and decoding process for multiple Skeleton fixtures. It verifies both object equality and JSON representation equality, which is thorough.
A suggestion for improvement:
Consider adding a check for the
encoded_json_strto ensure it's not empty, similar to the suggestion for the previous test:assert encoded_json_str, f"Encoded JSON string should not be empty for {skeleton_fixture_name}"This assertion could be added just after line 54.
Additionally, it might be beneficial to add a comment explaining the purpose of using
json.loadswhen comparing the JSON strings, as it normalizes the string representations for comparison.sleap/skeleton.py (1)
Line range hint
1211-1237: Remove debug print statements.The debug print statements added to the
to_jsonmethod are helpful for development and debugging but should be removed or commented out in production code. Consider using a logging framework for debugging in the future, which allows for easier management of debug output.Apply this diff to remove the debug print statements:
- print(f"indexed_node_graph: {indexed_node_graph}") - print(f"indexed_node_graph: {indexed_node_graph}") - print(f"graph: {graph}") - print(f"data: {data}") - print(f"data: {data}") - print(f"json_str: {json_str}")skeletons.ipynb (1)
44-379: Consider reducing debug output for clarityThe extensive debug output from the encoding process can clutter the notebook and make it less readable. Consider adjusting the logging level or redirecting debug information to a log file to improve clarity.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- environment.yml (1 hunks)
- environment_no_cuda.yml (1 hunks)
- skeletons.ipynb (1 hunks)
- sleap/skeleton.py (3 hunks)
- tests/test_skeleton.py (1 hunks)
Additional comments not posted (6)
environment_no_cuda.yml (1)
14-14: Approved: Explicit version constraint forattrspackageThe change from
- conda-forge::attrs >=21.2.0 #,<=21.4.0to- conda-forge::attrs >=21.2.0,<=21.4.0is a good improvement. It explicitly sets both lower and upper bounds for theattrspackage version, which helps ensure compatibility and reproducibility of the environment.Benefits of this change:
- Prevents potential issues with newer, untested versions of
attrs.- Ensures consistency across different installations of the project.
- Follows best practices for specifying dependency versions in conda environment files.
environment.yml (1)
13-13: Approve version constraint update forattrspackage.The change to specify both lower and upper bounds for the
attrspackage version is a good practice in dependency management. It helps ensure compatibility and stability across different environments.To ensure this change doesn't conflict with other dependencies, please run the following script:
This script will help identify any potential conflicts or usage patterns that might be affected by the version constraint update.
Verification successful
Approve version constraint update for
attrspackage.The updated version constraint
attrs >=21.2.0,<=21.4.0is compatible with the existing dependencies, includingcattrs ==1.1.1. This ensures stability and compatibility across the codebase without introducing conflicts.
- No conflicts detected with other dependencies.
attrsis consistently used across multiple modules, and the version range is appropriate.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify compatibility of attrs version with other dependencies # Test: Check if any other packages directly depend on attrs rg --type yaml -i 'attrs' environment.yml # Test: Look for any potential conflicts in other files rg --type python -i 'import attr' -A 5Length of output: 190
Script:
#!/bin/bash # Description: Verify compatibility of attrs version with other dependencies # Test: Check if any other packages directly depend on attrs rg --type yaml -i 'attrs' environment.yml # Test: Look for any potential conflicts in other files rg --type py -i 'import attr' -A 5Length of output: 19024
tests/test_skeleton.py (2)
5-5: LGTM: New imports are appropriate for the added tests.The new imports of
jsonandSkeletonEncoderare necessary for the newly added test functions. They are correctly placed and follow the existing import style.Also applies to: 9-9
1-67: Overall: Excellent addition of tests for SkeletonEncoder functionality.The new test functions
test_decoded_encoded_Skeleton_from_load_jsonandtest_decoded_encoded_Skeletonprovide thorough coverage for the newSkeletonEncoderfunctionality. They test both single file loading and multiple fixtures, ensuring robustness of the encoding and decoding process.The tests are well-structured, use appropriate assertions, and align well with the PR objectives. The parameterized test allows for easy expansion of test cases in the future.
Minor suggestions for improvement have been made to enhance error catching and clarity. These changes, while not critical, would further strengthen the test suite.
Great job on implementing these comprehensive tests!
sleap/skeleton.py (2)
88-289: LGTM: SkeletonEncoder implementation is complete and well-structured.The
SkeletonEncoderclass has been fully implemented, addressing the previous comment about adding the missing encoder implementation. The class provides a comprehensive set of methods for encoding various data types and objects, including a reference system for previously seen objects. The implementation follows good practices for custom JSON encoding.
1235-1236: LGTM: SkeletonEncoder integration in to_json method.The
to_jsonmethod has been successfully updated to use the newSkeletonEncoderinstead ofjsonpickle.encode. This change aligns with the goal of replacing the existing functionality with a custom encoder.
skeletons.ipynb
Outdated
| "# Save the encoded json string to a file\n", | ||
| "with open(\"encoded_json_str.json\", \"w\") as f:\n", | ||
| " f.write(encoded_json_str)" |
There was a problem hiding this comment.
Add exception handling when writing to a file
Incorporating exception handling when writing the encoded JSON string to a file can prevent the program from crashing due to unexpected I/O errors.
Apply this diff to add exception handling:
with open("encoded_json_str.json", "w") as f:
f.write(encoded_json_str)
+except IOError as e:
+ print(f"An error occurred while writing to 'encoded_json_str.json': {e}")Committable suggestion was skipped due to low confidence.
skeletons.ipynb
Outdated
| "# Get the skeleton from the encoded json string\n", | ||
| "decoded_skeleton = Skeleton.from_json(encoded_json_str)\n", | ||
| "decoded_skeleton" | ||
| ] |
There was a problem hiding this comment.
Add assertion to verify skeleton integrity after decoding
To ensure that the encoding and decoding processes maintain the skeleton's integrity, add an assertion to confirm that the decoded skeleton matches the original.
Apply this diff to add the assertion:
skeleton = Skeleton.load_json(fly_skeleton_legs_json)
decoded_skeleton = Skeleton.from_json(encoded_json_str)
+assert skeleton.matches(decoded_skeleton), "Decoded skeleton does not match the original."
decoded_skeletonCommittable suggestion was skipped due to low confidence.
skeletons.ipynb
Outdated
| "# def test_SkeletonEncoder(fly_legs_skeleton_json):\n", | ||
| "# \"\"\"\n", | ||
| "# Test SkeletonEncoder.encode method.\n", | ||
| "# \"\"\"\n", | ||
| "# # Get the skeleton from the fixture\n", | ||
| "# skeleton = Skeleton.load_json(fly_legs_skeleton_json)\n", | ||
| "# # Get the graph from the skeleton\n", | ||
| "# indexed_node_graph = skeleton._graph\n", | ||
| "# graph = json_graph.node_link_data(indexed_node_graph)\n", | ||
| "\n", | ||
| "# # Encode the graph as a json string to test .encode method\n", | ||
| "# encoder = SkeletonEncoder()\n", | ||
| "# encoded_json_str = encoder.encode(graph)\n", | ||
| "\n", | ||
| "# # Get the skeleton from the encoded json string\n", | ||
| "# decoded_skeleton = Skeleton.from_json(encoded_json_str)\n", | ||
| "\n", | ||
| "# # Check that the decoded skeleton is the same as the original skeleton\n", | ||
| "# assert skeleton.matches(decoded_skeleton)" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uncomment and integrate the test for SkeletonEncoder
The test function for SkeletonEncoder.encode is currently commented out. Enabling this test would automate verification of the encoding and decoding processes, ensuring data integrity.
Apply this diff to uncomment and update the test function:
-# def test_SkeletonEncoder(fly_legs_skeleton_json):
-# """
-# Test SkeletonEncoder.encode method.
-# """
-# # Get the skeleton from the fixture
-# skeleton = Skeleton.load_json(fly_legs_skeleton_json)
-# # Get the graph from the skeleton
-# indexed_node_graph = skeleton._graph
-# graph = json_graph.node_link_data(indexed_node_graph)
-
-# # Encode the graph as a json string to test .encode method
-# encoder = SkeletonEncoder()
-# encoded_json_str = encoder.encode(graph)
-
-# # Get the skeleton from the encoded json string
-# decoded_skeleton = Skeleton.from_json(encoded_json_str)
-
-# # Check that the decoded skeleton is the same as the original skeleton
-# assert skeleton.matches(decoded_skeleton)
+def test_SkeletonEncoder():
+ """
+ Test SkeletonEncoder.encode method.
+ """
+ # Get the skeleton from the fixture
+ fly_legs_skeleton_json = "tests/data/skeleton/fly_skeleton_legs.json"
+ skeleton = Skeleton.load_json(fly_legs_skeleton_json)
+ # Get the graph from the skeleton
+ indexed_node_graph = skeleton._graph
+ graph = json_graph.node_link_data(indexed_node_graph)
+
+ # Encode the graph as a json string to test .encode method
+ encoder = SkeletonEncoder()
+ encoded_json_str = encoder.encode(graph)
+
+ # Get the skeleton from the encoded json string
+ decoded_skeleton = Skeleton.from_json(encoded_json_str)
+
+ # Check that the decoded skeleton is the same as the original skeleton
+ assert skeleton.matches(decoded_skeleton)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.
| "# def test_SkeletonEncoder(fly_legs_skeleton_json):\n", | |
| "# \"\"\"\n", | |
| "# Test SkeletonEncoder.encode method.\n", | |
| "# \"\"\"\n", | |
| "# # Get the skeleton from the fixture\n", | |
| "# skeleton = Skeleton.load_json(fly_legs_skeleton_json)\n", | |
| "# # Get the graph from the skeleton\n", | |
| "# indexed_node_graph = skeleton._graph\n", | |
| "# graph = json_graph.node_link_data(indexed_node_graph)\n", | |
| "\n", | |
| "# # Encode the graph as a json string to test .encode method\n", | |
| "# encoder = SkeletonEncoder()\n", | |
| "# encoded_json_str = encoder.encode(graph)\n", | |
| "\n", | |
| "# # Get the skeleton from the encoded json string\n", | |
| "# decoded_skeleton = Skeleton.from_json(encoded_json_str)\n", | |
| "\n", | |
| "# # Check that the decoded skeleton is the same as the original skeleton\n", | |
| "# assert skeleton.matches(decoded_skeleton)" | |
| def test_SkeletonEncoder(): | |
| """ | |
| Test SkeletonEncoder.encode method. | |
| """ | |
| # Get the skeleton from the fixture | |
| fly_legs_skeleton_json = "tests/data/skeleton/fly_skeleton_legs.json" | |
| skeleton = Skeleton.load_json(fly_legs_skeleton_json) | |
| # Get the graph from the skeleton | |
| indexed_node_graph = skeleton._graph | |
| graph = json_graph.node_link_data(indexed_node_graph) | |
| # Encode the graph as a json string to test .encode method | |
| encoder = SkeletonEncoder() | |
| encoded_json_str = encoder.encode(graph) | |
| # Get the skeleton from the encoded json string | |
| decoded_skeleton = Skeleton.from_json(encoded_json_str) | |
| # Check that the decoded skeleton is the same as the original skeleton | |
| assert skeleton.matches(decoded_skeleton) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
tests/test_skeleton.py (1)
1-10: Organize imports according to PEP 8 guidelinesThe new imports have been added correctly, but we can improve the overall organization of the import statements to follow PEP 8 guidelines more closely. This will enhance readability and maintainability.
Consider reorganizing the imports as follows:
import copy import json import os import jsonpickle import pytest from networkx.readwrite import json_graph from sleap.skeleton import Skeleton, SkeletonEncoderThis organization:
- Groups standard library imports first.
- Separates third-party library imports with a blank line.
- Places local imports (from the
sleappackage) last.- Combines imports from the same module on a single line.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- sleap/skeleton.py (4 hunks)
- tests/test_skeleton.py (1 hunks)
🔇 Additional comments not posted (11)
tests/test_skeleton.py (1)
1-60: Overall assessment: Well-implemented tests for SkeletonEncoderThe new test functions
test_decoded_encoded_Skeleton_from_load_jsonandtest_decoded_encoded_Skeletonare valuable additions to the test suite. They effectively verify the functionality of the new SkeletonEncoder and ensure that Skeleton objects can be correctly encoded and decoded.Key strengths:
- Comprehensive testing of encoding and decoding for different Skeleton objects.
- Use of parameterized testing to cover multiple scenarios.
- Verification of both object equality and JSON representation consistency.
The suggested improvements in the previous comments will further enhance the robustness and clarity of these tests. Great job on expanding the test coverage!
sleap/skeleton.py (10)
88-121: LGTM: SkeletonEncoder class initialization and docstring.The class docstring provides a clear explanation of the purpose and functionality of the
SkeletonEncoder. The__init__method initializes the_encoded_objectsdictionary to manage object references during encoding.
123-136: LGTM: encode class method implementation.The
encodeclass method provides a clean interface for encoding data. It creates an instance of the encoder, calls the private_encodemethod, and then usesjson.dumpsto create the final JSON string.
138-167: LGTM: _encode method implementation.The
_encodemethod handles different types of objects (dict, list, EdgeType, Node) appropriately. It includes special handling for dictionaries containing 'nodes' and 'links' keys, which is likely specific to the skeleton structure.
169-206: LGTM: _encode_links method implementation.The
_encode_linksmethod ensures that the links (edges) are encoded in a specific order (source, target, type, other attributes). This maintains consistency in the JSON output.
208-235: LGTM: _encode_node method implementation.The
_encode_nodemethod handles bothNodeobjects and integer indices. It checks for previous encoding to avoid redundancy and uses a specific format for encodingNodeobjects.
237-260: LGTM: _encode_edge_type method implementation.The
_encode_edge_typemethod properly encodesEdgeTypeobjects, handling both first-time encoding and references to previously encoded objects.
262-278: LGTM: _get_or_assign_id method implementation.This method manages the assignment of
py/idfor objects, ensuring unique identification and handling both new and previously encoded objects.
280-291: LGTM: _is_first_encoding method implementation.This simple method checks if an object is being encoded for the first time, which is crucial for the reference system used in the encoder.
1146-1146: LGTM: Updated to_json method using SkeletonEncoder.The
to_jsonmethod has been updated to use the newSkeletonEncoderclass. The changes include:
- Using
SkeletonEncoder.encode(data)instead ofjsonpickle.encode(data).- Proper handling of
node_to_idxmapping when provided.- Maintaining backwards compatibility by only including
descriptionandpreview_imagefields for template skeletons.These changes improve the JSON encoding process while maintaining compatibility with existing data formats.
Also applies to: 1210-1213, 1232-1232
Line range hint
88-1232: Overall implementation of SkeletonEncoder and integration with Skeleton class is well-done.The new
SkeletonEncoderclass provides a custom JSON encoding solution for theSkeletonclass, replacing the previousjsonpickle.encodefunctionality. The implementation is thorough, handling various object types and maintaining a reference system for efficient encoding. The integration with theSkeletonclass, particularly in theto_jsonmethod, is done smoothly while maintaining backwards compatibility.Key improvements:
- Custom encoding for
NodeandEdgeTypeobjects.- Efficient handling of repeated objects using a reference system.
- Maintaining order of attributes in encoded links.
- Backwards compatibility for non-template skeletons.
The changes should result in more efficient and controlled JSON encoding for
Skeletonobjects without breaking existing functionality.
| def test_decoded_encoded_Skeleton_from_load_json(fly_legs_skeleton_json): | ||
| """ | ||
| Test Skeleton decoded from SkeletonEncoder.encode matches the original Skeleton. | ||
| """ | ||
| # Get the skeleton from the fixture | ||
| skeleton = Skeleton.load_json(fly_legs_skeleton_json) | ||
| # Get the graph from the skeleton | ||
| indexed_node_graph = skeleton._graph | ||
| graph = json_graph.node_link_data(indexed_node_graph) | ||
|
|
||
| # Encode the graph as a json string to test .encode method | ||
| encoded_json_str = SkeletonEncoder.encode(graph) | ||
|
|
||
| # Get the skeleton from the encoded json string | ||
| decoded_skeleton = Skeleton.from_json(encoded_json_str) | ||
|
|
||
| # Check that the decoded skeleton is the same as the original skeleton | ||
| assert skeleton.matches(decoded_skeleton) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional assertions
The test function effectively verifies that the encoded and decoded Skeleton matches the original. However, we can improve it by adding more specific assertions:
- Assert that the encoded JSON string is not empty.
- Compare the number of nodes and edges in the original and decoded skeletons.
- Verify that the node names and edge connections are preserved.
Consider adding these assertions to strengthen the test:
def test_decoded_encoded_Skeleton_from_load_json(fly_legs_skeleton_json):
skeleton = Skeleton.load_json(fly_legs_skeleton_json)
indexed_node_graph = skeleton._graph
graph = json_graph.node_link_data(indexed_node_graph)
encoded_json_str = SkeletonEncoder.encode(graph)
assert encoded_json_str, "Encoded JSON string should not be empty"
decoded_skeleton = Skeleton.from_json(encoded_json_str)
assert skeleton.matches(decoded_skeleton)
assert len(skeleton.nodes) == len(decoded_skeleton.nodes), "Number of nodes should match"
assert len(skeleton.edges) == len(decoded_skeleton.edges), "Number of edges should match"
assert set(n.name for n in skeleton.nodes) == set(n.name for n in decoded_skeleton.nodes), "Node names should match"
assert set(skeleton.edge_names) == set(decoded_skeleton.edge_names), "Edge connections should match"| @pytest.mark.parametrize( | ||
| "skeleton_fixture_name", ["flies13_skeleton", "skeleton", "stickman"] | ||
| ) | ||
| def test_decoded_encoded_Skeleton(skeleton_fixture_name, request): | ||
| """ | ||
| Test Skeleton decoded from SkeletonEncoder.encode matches the original Skeleton. | ||
| """ | ||
| # Use request.getfixturevalue to get the actual fixture value by name | ||
| skeleton = request.getfixturevalue(skeleton_fixture_name) | ||
|
|
||
| # Get the graph from the skeleton | ||
| indexed_node_graph = skeleton._graph | ||
| graph = json_graph.node_link_data(indexed_node_graph) | ||
|
|
||
| # Encode the graph as a json string to test .encode method | ||
| encoded_json_str = SkeletonEncoder.encode(graph) | ||
|
|
||
| # Get the skeleton from the encoded json string | ||
| decoded_skeleton = Skeleton.from_json(encoded_json_str) | ||
|
|
||
| # Check that the decoded skeleton is the same as the original skeleton | ||
| assert skeleton.matches(decoded_skeleton) | ||
|
|
||
| # Now make everything into a JSON string | ||
| skeleton_json_str = skeleton.to_json() | ||
| decoded_skeleton_json_str = decoded_skeleton.to_json() | ||
|
|
||
| # Check that the JSON strings are the same | ||
| assert json.loads(skeleton_json_str) == json.loads(decoded_skeleton_json_str) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance parameterized test with additional assertions and error messages
The test function effectively verifies that the encoded and decoded Skeleton matches the original across multiple fixtures. To further improve its robustness:
- Add assertions for the number of nodes and edges.
- Verify that node names and edge connections are preserved.
- Include more descriptive error messages in assertions.
Consider enhancing the test function as follows:
@pytest.mark.parametrize(
"skeleton_fixture_name", ["flies13_skeleton", "skeleton", "stickman"]
)
def test_decoded_encoded_Skeleton(skeleton_fixture_name, request):
skeleton = request.getfixturevalue(skeleton_fixture_name)
indexed_node_graph = skeleton._graph
graph = json_graph.node_link_data(indexed_node_graph)
encoded_json_str = SkeletonEncoder.encode(graph)
assert encoded_json_str, f"Encoded JSON string for {skeleton_fixture_name} should not be empty"
decoded_skeleton = Skeleton.from_json(encoded_json_str)
assert skeleton.matches(decoded_skeleton), f"Decoded {skeleton_fixture_name} should match the original"
assert len(skeleton.nodes) == len(decoded_skeleton.nodes), f"Number of nodes in {skeleton_fixture_name} should match"
assert len(skeleton.edges) == len(decoded_skeleton.edges), f"Number of edges in {skeleton_fixture_name} should match"
assert set(n.name for n in skeleton.nodes) == set(n.name for n in decoded_skeleton.nodes), f"Node names in {skeleton_fixture_name} should match"
assert set(skeleton.edge_names) == set(decoded_skeleton.edge_names), f"Edge connections in {skeleton_fixture_name} should match"
skeleton_json_str = skeleton.to_json()
decoded_skeleton_json_str = decoded_skeleton.to_json()
assert json.loads(skeleton_json_str) == json.loads(decoded_skeleton_json_str), f"JSON representations of {skeleton_fixture_name} should match"These changes will provide more detailed information if a test fails, making it easier to identify and fix issues.
Description
Looking at our
fly_skeleton_legs.jsontest data:Our final
json_strhas keysdirected,graph,links,multigraph,nodes.directedis a boolean. (UNCHANGED from input graph)graphis a dict with keysnameandnum_edges_inserted. (UNCHANGED from input graph)linksis a list of dicts with keysedge_insert_idx,key,source,target,type, for each edge.multigraphis a boolean = True. (UNCHANGED from input graph)nodesis a list of dicts with keysidfor each node.Node(name='neck', weight=1.0)"links":[{... source: { Node(name='neck', weight=1.0) }{"py/object": "sleap.skeleton.Node", "py/state": {"py/tuple": ["neck", 1.0]}}Node(name='head', weight=1.0)"links":[{... target: { Node(name='head', weight=1.0) }{"py/object": "sleap.skeleton.Node", "py/state": {"py/tuple": ["head", 1.0]}}<EdgeType.BODY: 1>"links":[{... type: { <EdgeType.BODY: 1> }{"py/reduce": [{"py/type": "sleap.skeleton.EdgeType"}, {"py/tuple": [1]}, null, null, null]}Node(name='head', weight=1.0)"nodes": [{'id': Node(name='head', weight=1.0)}, ..., ]{"id": {"py/id": 2}}Skeleton._graphis passed throughjson_graph.node_link_data, which returns a dictionary with node-link formatted data (see documentation here.linksand notlink.networkxpinned. I am not sure why that hasn't been a problem.jsonpickle.encode(data).Node. EachNodeobject, which has attributesnameandweight, is encoded as a Python objectsleap.skeleton.Node. TheNode's internal state is represented as a Python tuple containing thename(as a string) andweight(as a numerical value):["name", weight].node_to_idxmapping (sleap/sleap/skeleton.py
Lines 1002 to 1006 in 3c7f5af
Labels, the node is returned as an integer without the name and weight attributes.EdgeType.EdgeTypeinherits from Python'sEnumclass. The possible edge types areBODY = 1andSYMMETRY = 2. When serialized,EdgeTypeis represented using Python'sreducefunction, storing the type assleap.skeleton.EdgeTypeand the value as a Python tuple containing theEnumvalue.py/id, which starts at 1 and indexes the objects in the order they are seen so that the second time the first object is used, it will be referenced as{"py/id": 1}.sourceortargetin thelinkslist, it is encoded as a py/object with a py/state. Then it is referenced later on using the py/id.nodesthen uses only the py/id to reference theNode.jsongraph.node_link_data()tojsonpickle.encode()(shown below) which hasnodesfirst andlinksafter.source,targetandtype: in the graph belowtypecomes beforesourceandtargetso unless explicitly ordered aftersourceandtargetthe py/id will not be consistent with the legacy data.SkeletonEncodercalled_encode_linksthat is used within the classmethodencodeto encode thelinksfirst, and within thelinkslist, encode thesource,targetand then thetype.EdgeTypewould be decoded as aNodeand the edges would not be formed correctly in the decodedSkeletonusingSkeleton.from_json.json_graph.node_link_datafromnetworkx, encodes the graph with the newSkeletonEncoder.encode()method, then usesSkeleton.from_jsonto deserialize the JSON string (withjsonpickle.decode, thenjson_graph.node_link_graph). TheSkeleton.matches()method is used to test the equivalence of the deserializedSkeletons.[X] This test could be parametrized to do with various skeleton fixtures.
[X] We should make sure to do this with a template fixture at least.
[X] An additional test could be added to see if the serialized JSON strings are equivalent.
attrsis unconstrained (see github run) because with the version ofattrspulled in, the nodes arejson_picklefails to decode theNodes which have been encoded as tuples by ourSkeletonEncoder.encode()(to maintain backwards compatibility).__setstate__from "legacy" tuple format soon python-attrs/attrs#1091) is to remove support for deserializing tuples after giving everyone a few versions of attrs to convert their data over to a dictionary serialization.""" from Error during Inference. ValueError: Found Multiple nodes named (name). #1918 .Types of changes
Does this address any currently open issues?
#1470 #1918
This pull request accompanies #1961 in handling JSON encoding and decoding internally instead of relying on
jsonpickle.Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
Summary by CodeRabbit
New Features
SkeletonEncoderclass for enhanced encoding of skeleton representations.NodeandEdgeTypeobjects into JSON formats.Chores
attrspackage in environment configuration files.Tests
Skeletonobjects.