Handle skeleton decoding internally #1961
Conversation
WalkthroughThe changes encompass the introduction of a new Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
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 (
|
i.e. loading from Labels (slp file)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1961 +/- ##
===========================================
+ Coverage 73.30% 75.40% +2.09%
===========================================
Files 134 133 -1
Lines 24087 24552 +465
===========================================
+ Hits 17658 18514 +856
+ Misses 6429 6038 -391 ☔ View full report in Codecov by Sentry. |
roomrys
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
sleap/skeleton.py (1)
310-313: Consider combining theifbranches using logicaloroperator.The static analysis hint suggests combining the
ifbranches in the_decode_reduce_dictmethod using the logicaloroperator. This would improve readability without changing the functionality.You can apply this diff to combine the
ifbranches:- if reduce_item is None: - # Sometimes the reduce list has None values, skip them - continue - if ( - "py/type" in reduce_item - and reduce_item["py/type"] == "sleap.skeleton.EdgeType" - ): + if reduce_item is None or ( + "py/type" in reduce_item and + reduce_item["py/type"] == "sleap.skeleton.EdgeType" + ):Tools
Ruff
310-313: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- sleap/gui/widgets/docks.py (2 hunks)
- sleap/skeleton.py (3 hunks)
- sleap/util.py (0 hunks)
- tests/data/skeleton/fly_skeleton_legs_pystate_dict.json (1 hunks)
- tests/fixtures/skeletons.py (1 hunks)
- tests/test_skeleton.py (4 hunks)
- tests/test_util.py (0 hunks)
Files not reviewed due to no reviewable changes (2)
- sleap/util.py
- tests/test_util.py
Additional context used
Ruff
sleap/skeleton.py
310-313: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
Additional comments not posted (11)
tests/fixtures/skeletons.py (2)
11-14: LGTM!The updated docstring clarifies that the JSON file contains skeleton data in tuple format, which improves the understanding of the fixture's purpose.
18-24: Great addition!The new
fly_legs_skeleton_dict_jsonfixture enhances testing capabilities by providing skeleton data in dictionary format. The docstring clearly explains the purpose and format of the data.tests/data/skeleton/fly_skeleton_legs_pystate_dict.json (1)
1-1: LGTM!The addition of this test fixture file is a good practice to ensure the robustness of the skeleton decoding functionality. The JSON structure follows a specific schema to represent the skeleton data and serves as a valid input for testing purposes.
tests/test_skeleton.py (5)
196-198: LGTM!The change in the decoding method from
jsonpickle.decodetoSkeletonDecoder.decodeis consistent with the expected behavior mentioned in the AI-generated summary. The assertion has been updated accordingly to verify the presence of the "nx_graph" key in the decoded JSON dictionary.
210-212: LGTM!The code segment correctly tests the behavior of the
SkeletonDecoderwhenis_templateis set toTrue. The assertions verify the presence of the expected keys in the decoded JSON dictionary, which is consistent with the changes mentioned in the AI-generated summary.
226-230: LGTM!The addition of the
test_decode_preview_imagefunction is a valuable test case for verifying the functionality of the newly addeddecode_preview_imagefunction in theSkeletonDecoderclass. The assertion ensures that the decoded preview image is in the expected "RGBA" mode, which is consistent with the changes mentioned in the AI-generated summary.
233-243: LGTM!The addition of the
test_skeleton_decoderfunction is a valuable test case for verifying that theSkeletonDecodercan handle both tuple and dictionary formats for skeleton data. The assertions ensure that the loaded objects are instances of theSkeletonclass and that they match each other, which is consistent with the changes mentioned in the AI-generated summary.
2-6: LGTM!The addition of the
osandSkeletonDecoderimport statements is necessary for the changes made in the test suite. Theosimport is required for file-related operations, and theSkeletonDecoderimport is needed for using the newly addedSkeletonDecoderclass in the test functions. These changes are consistent with the AI-generated summary.sleap/gui/widgets/docks.py (1)
366-366: Approve the usage ofSkeletonDecoder.decode_preview_image.The change from using the
decode_preview_imagefunction to theSkeletonDecoder.decode_preview_imagemethod aligns with the PR objective of internalizing the (de)serialization process. This refactoring helps reduce dependency on external libraries and improves the maintainability of the codebase.Verify that all usages of
decode_preview_imagehave been replaced withSkeletonDecoder.decode_preview_imagein the codebase by running the following script:Verification successful
Refactoring of
decode_preview_imagetoSkeletonDecoder.decode_preview_imageis successful.All instances of
decode_preview_imagehave been correctly replaced withSkeletonDecoder.decode_preview_imageacross the codebase, aligning with the refactoring objectives. However, the originaldecode_preview_imagefunction definition remains insleap/skeleton.py. Consider reviewing its necessity to ensure it is not redundant.
sleap/skeleton.py: Definition ofdecode_preview_imagestill exists.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `decode_preview_image` have been replaced. # Test 1: Search for usages of `decode_preview_image`. Expect: No results. rg --type python $'decode_preview_image' # Test 2: Search for usages of `SkeletonDecoder.decode_preview_image`. Expect: At least 1 result. rg --type python $'SkeletonDecoder.decode_preview_image'Length of output: 157
Script:
#!/bin/bash # Description: Verify all usages of `decode_preview_image` have been replaced. # Test 1: Search for usages of `decode_preview_image`. Expect: No results. rg 'decode_preview_image' # Test 2: Search for usages of `SkeletonDecoder.decode_preview_image`. Expect: At least 1 result. rg 'SkeletonDecoder.decode_preview_image'Length of output: 767
sleap/skeleton.py (2)
89-377: Great work on introducing theSkeletonDecoderclass!The class is well-structured, provides a clear separation of concerns for decoding different components, and includes helpful docstrings. This custom decoding mechanism is a good step towards internalizing the (de)serialization process within the SLEAP framework.
Tools
Ruff
310-313: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
1365-1365: LGTM!The change to use
SkeletonDecoder.decodeinstead ofjsonpickle.decodein thefrom_jsonmethod aligns well with the objective of internalizing the decoding process. The modification is straightforward and looks good.
Description
It has become apparent that it will be safer for us to handle (de)serialization of our data structures internally rather than relying on external libraries such as
cattrandjsonpickleto do this for us. Without constraining these (de)serialization dependencies (which may also cause other upgrade/dependency constraints elsewhere), we cannot expect that the format of (de)serialization will be backwards compatible within (even the same!) version of SLEAP (albeit installed at different times).Keeping with the theme above, this PR is the first of many that starts migrating (de)serialization to internal SLEAP code. In this PR, we add a function to replace
jsonpickle.decodewhich is intended to be used onjsonpickle.encodedSkeletons.Types of changes
Does this address any currently open issues?
skeleton.to_jsonto serialize skeleton object without jsonpickle #1934Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
New Features
SkeletonDecoderclass for enhanced decoding of skeleton data and preview images.Bug Fixes
Tests
SkeletonDecoderand decoding preview images to ensure functionality.