-
Notifications
You must be signed in to change notification settings - Fork 96
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
Sort encoded Skeleton
dictionary for backwards compatibility
#1975
Sort encoded Skeleton
dictionary for backwards compatibility
#1975
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1975 +/- ##
===========================================
+ Coverage 73.30% 75.48% +2.17%
===========================================
Files 134 133 -1
Lines 24087 24635 +548
===========================================
+ Hits 17658 18596 +938
+ Misses 6429 6039 -390 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/test_skeleton.py (1)
48-59
: LGTM! Comprehensive check for sorted keys enhances backwards compatibility.The added assertions effectively ensure that the encoded JSON has keys in sorted order, which aligns well with the PR objective of maintaining backwards compatibility. The implementation is thorough, covering nested dictionaries and lists.
Consider renaming the unused loop variable
key
to_
to indicate it's intentionally unused:-for key, value in encoded_dict.items(): +for _, value in encoded_dict.items():Would you like assistance in generating additional test cases to cover edge cases or specific backwards compatibility scenarios?
🧰 Tools
🪛 Ruff
52-52: Loop control variable
key
not used within loop bodyRename unused
key
to_key
(B007)
sleap/skeleton.py (2)
423-428
: LGTM! Consider adding a version check for optimization.The addition of sorting the input data addresses the backwards compatibility issue with SLEAP <=1.3.4 as mentioned in the PR objectives. This change is well-implemented and commented.
For potential optimization, you might consider adding a version check to skip the sorting step for newer SLEAP versions. This could look like:
if sleap_version <= Version("1.3.4"): sorted_data = cls._recursively_sort_dict(data) else: sorted_data = dataThis would maintain backwards compatibility while potentially improving performance for newer versions.
432-446
: LGTM! Consider adding a type check for potential optimization.The
_recursively_sort_dict
method is well-implemented and correctly handles nested structures. It effectively addresses the need for consistent ordering in the encoded data, which is crucial for backwards compatibility.For a potential optimization, you might consider adding a type check before recursing into list items. This could prevent unnecessary function calls for lists that don't contain dictionaries:
elif isinstance(value, list): if any(isinstance(item, dict) for item in value): for i, item in enumerate(value): if isinstance(item, dict): sorted_dict[key][i] = SkeletonEncoder._recursively_sort_dict(item)This change would skip the recursion for lists that don't contain any dictionaries, potentially improving performance for certain data structures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- sleap/skeleton.py (2 hunks)
- tests/test_skeleton.py (1 hunks)
🧰 Additional context used
🪛 Ruff
tests/test_skeleton.py
52-52: Loop control variable
key
not used within loop bodyRename unused
key
to_key
(B007)
🔇 Additional comments (1)
sleap/skeleton.py (1)
423-446
: Summary: Changes effectively address backwards compatibility.The modifications to the
SkeletonEncoder
class, particularly the addition of the_recursively_sort_dict
method and its use in theencode
method, successfully address the backwards compatibility issue with SLEAP versions <=1.3.4. The implementation is clean, well-commented, and maintains the overall structure of the class.These changes align well with the PR objectives, solving the issue of loading
Skeleton
JSONs across different versions of the SLEAP framework. The consistent ordering of dictionary keys ensures that the encoded data remains compatible with older versions that rely onjsonpickle.decode
.The implementation is solid, and with the suggested minor optimizations, it should provide both backwards compatibility and good performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subgroup approves
Description
In #1970, we created our own
SkeletonEncoder
class to move away from usingjsonpickle.encode
. And, while theSkeletonEncoder
paired with theSkeletonDecoder
works perfectly fine, we get some strange behavior when trying to loadSkeleton
jsons across versions of SLEAP (that still usejsonpickle.decode
).The only difference seemed to be the ordering of the keys in the dictionaries; thus, this PR sorts the
Skeleton
dictionaries before encoding them - which has proven to do the trick with some manual testing across versions.Unsorted skeleton (breaks backwards compatibility)
Sorted skeleton (backwards compatible)
v1.3.3 Skeleton
Types of changes
Does this address any currently open issues?
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit