Small improvements to tracking #1894
Conversation
WalkthroughThe recent updates to the Sleap framework enhance tracking and inference capabilities. Key modifications include refined configuration options, improved error handling, and better user experience. Changes optimize performance, introduce new reporting features, and streamline object-oriented design, making the system more maintainable and extendable. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- sleap/config/pipeline_form.yaml (2 hunks)
- sleap/gui/learning/dialog.py (1 hunks)
- sleap/gui/widgets/video.py (2 hunks)
- sleap/nn/inference.py (12 hunks)
- sleap/nn/tracking.py (15 hunks)
- sleap/util.py (2 hunks)
- tests/nn/test_tracker_components.py (2 hunks)
- tests/nn/test_tracking_integration.py (4 hunks)
Additional context used
Ruff
sleap/util.py
38-38: Undefined name
Task(F821)
sleap/nn/tracking.py
6-6:
collections.defaultdictimported but unusedRemove unused import:
collections.defaultdict(F401)
614-614: Use
enumerate()for index variablen_processedinforloop(SIM113)
Additional comments not posted (21)
tests/nn/test_tracking_integration.py (3)
22-22: Verify the new expected value in the assertion.The assertion has been changed to expect 8 tracks instead of 27. Ensure that this new expected value is correct based on the updated tracking logic or data.
44-44: Verify thetracker.run_trackermethod invocation.Ensure that the
trackerobject is correctly instantiated and that therun_trackermethod handles the verbosity parameter appropriately.
51-51: Verify thetracker.run_trackermethod invocation.Ensure that the
trackerobject is correctly instantiated and that therun_trackermethod handles the verbosity parameter appropriately.tests/nn/test_tracker_components.py (2)
18-27: Verify the handling of theimg_scaleparameter and the test skipping logic.Ensure that the
img_scaleparameter is correctly applied to thecandidate_makerof the tracker and that the function appropriately skips tests whenimg_scaleis not defined.
47-58: Verify the parameterized tests and the passing of theimg_scaleparameter.Ensure that the parameterized tests are correctly implemented and that the
img_scaleparameter is passed appropriately to therun_tracker_by_namefunction.sleap/config/pipeline_form.yaml (2)
458-463: New configuration optiontracking.save_shifted_instancesadded.This option is well-documented and the default value is set to
true, which aligns with the PR summary indicating the enhancement of instance matching.
457-457: Verify the impact of removingtracking.max_tracking.Ensure that the removal of this configuration option does not negatively impact any existing functionality or user workflows.
Verification successful
Verify the integration of
tracking.max_tracksand check for any remaining references totracking.max_tracking.Ensure that
tracking.max_tracksis correctly integrated into the codebase and that there are no remaining dependencies ontracking.max_tracking.
The integration of
tracking.max_tracksis correct and there are no remaining dependencies ontracking.max_tracking.The replacement of
tracking.max_trackingwithtracking.max_trackshas been successfully integrated into the configuration file, and there are no lingering references to the old configuration.
tracking.max_tracksis correctly defined and used in thepipeline_form.yamlfile.- No remaining references to
tracking.max_trackingwere found in non-YAML files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing `tracking.max_tracking`. # Test: Search for the usage of `tracking.max_tracking`. Expect: No occurrences. rg --type yaml -A 5 $'tracking.max_tracking'Length of output: 421
Script:
#!/bin/bash # Description: Verify the integration of `tracking.max_tracks` and check for any remaining references to `tracking.max_tracking`. # Test 1: Search for the usage of `tracking.max_tracks`. Expect: Relevant occurrences showing correct integration. rg --type yaml -A 5 $'tracking.max_tracks' # Test 2: Search for any remaining references to `tracking.max_tracking` in non-YAML files. Expect: No occurrences. rg --type yaml -v $'tracking.max_tracking'Length of output: 138158
sleap/gui/learning/dialog.py (1)
739-739: Enhanced robustness inrunmethod.The calculation of
no_result_countnow ensures it is non-negative, preventing potential errors in the inference process.sleap/nn/tracking.py (7)
142-142: Verify the impact of changing the default value ofsave_shifted_instances.Changing the default value of
save_shifted_instancestoTruecould impact memory usage and performance. Ensure that this change is intentional and tested.
532-548: LGTM!The new method
run_stepencapsulates the logic for running a single step of tracking.
549-653: LGTM!The new method
run_trackerencapsulates the tracking logic and enhances reporting capabilities based on verbosity levels.Tools
Ruff
614-614: Use
enumerate()for index variablen_processedinforloop(SIM113)
1707-1707: LGTM!The modification aligns with the changes in the
BaseTrackerclass.
520-521: LGTM! Verify the usage of new attributes.The new attributes
verbosityandreport_rateenhance the reporting capabilities.Ensure that these attributes are correctly used and tested throughout the codebase.
Verification successful
Let's modify the script to search for the usage of
verbosityandreport_ratein all Python files without specifying the file type.
The new attributes
verbosityandreport_rateare correctly used and tested throughout the codebase.The attributes are integrated into the main functionality and are covered by tests, ensuring their proper usage.
sleap/nn/tracking.pysleap/nn/inference.pytests/nn/test_tracking_integration.pytests/nn/test_inference.pytests/nn/test_tracker_components.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new attributes `verbosity` and `report_rate`. # Test: Search for the usage of `verbosity` and `report_rate`. Expect: Occurrences of the new attributes. rg --type python 'verbosity|report_rate'Length of output: 73
Script:
#!/bin/bash # Description: Verify the usage of new attributes `verbosity` and `report_rate`. # Test: Search for the usage of `verbosity` and `report_rate`. Expect: Occurrences of the new attributes. rg 'verbosity|report_rate' --glob '*.py'Length of output: 3311
742-747: LGTM! Verify the usage of new attributes.The new attributes
verbosityandreport_rateenhance the reporting capabilities.Ensure that these attributes are correctly used and tested throughout the codebase.
Verification successful
The new attributes
verbosityandreport_rateare correctly used and tested throughout the codebase.The search results indicate that both attributes are utilized in various parts of the code, including the
sleap/nn/tracking.py,sleap/nn/inference.py, and several test files. This confirms that the new attributes are integrated and tested.
- Files with relevant usage:
sleap/nn/tracking.pysleap/nn/inference.pytests/nn/test_tracking_integration.pytests/nn/test_tracker_components.pytests/nn/test_inference.pysleap/gui/learning/runners.pydocs/notebooks/Training_and_inference_on_an_example_dataset.ipynbdocs/guides/cli.mdScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new attributes `verbosity` and `report_rate`. # Test: Search for the usage of `verbosity` and `report_rate`. Expect: Occurrences of the new attributes. rg --type python 'verbosity|report_rate'Length of output: 73
Script:
#!/bin/bash # Description: Verify the usage of new attributes `verbosity` and `report_rate`. # Test: Search for the usage of `verbosity` and `report_rate` in Python files. rg --type-add 'py:*.py' 'verbosity|report_rate'Length of output: 5005
1484-1489: LGTM! Verify the usage of new attributes.The new attributes
verbosityandreport_rateenhance the reporting capabilities.Ensure that these attributes are correctly used and tested throughout the codebase.
Verification successful
The new attributes
verbosityandreport_rateare correctly used and tested throughout the codebase.The attributes are integrated into multiple files, including
sleap/nn/tracking.py,sleap/nn/inference.py, and various test files, ensuring their proper utilization and testing.
- Files with usage:
sleap/nn/tracking.pysleap/nn/inference.pytests/nn/test_tracker_components.pytests/nn/test_inference.pytests/nn/test_tracking_integration.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new attributes `verbosity` and `report_rate`. # Test: Search for the usage of `verbosity` and `report_rate`. Expect: Occurrences of the new attributes. rg --type python 'verbosity|report_rate'Length of output: 73
Script:
#!/bin/bash # Description: Verify the usage of new attributes `verbosity` and `report_rate`. # Test: Search for the usage of `verbosity` and `report_rate`. Expect: Occurrences of the new attributes. rg --type py 'verbosity|report_rate'Length of output: 3307
sleap/gui/widgets/video.py (2)
807-808: Initialization of_down_posis appropriate.The
_down_posattribute is correctly initialized toNoneto ensure it is defined before use.
1032-1032: Robustness improved with_down_poscheck.The check for
_down_posbeingNonebefore comparing it with the current mouse position enhances the robustness of the click detection logic.sleap/nn/inference.py (4)
48-51: Conditional import forcached_propertyis appropriate.The conditional import ensures compatibility with Python versions below 3.8, which is a good practice.
163-165: Use ofcached_propertyforreport_periodis appropriate.The
cached_propertydecorator improves performance by caching the computed value ofreport_period.
5341-5343: Inclusion ofexample_indicesinLabelsReaderinitialization is beneficial.The modification enhances the configurability by allowing more flexible handling of input frames based on user-defined indices.
5437-5440: Inclusion ofprogress_reportingin tracker instantiation is beneficial.The modification allows for more detailed tracking feedback, enhancing the monitoring of the tracking process.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/nn/tracking.py (15 hunks)
Additional context used
Ruff
sleap/nn/tracking.py
614-614: Use
enumerate()for index variablen_processedinforloop(SIM113)
Additional comments not posted (12)
sleap/nn/tracking.py (12)
4-5: Imports approved.The additions of
json,sys, andrich.progressare appropriate for the new reporting functionality.
520-521: New attributesverbosityandreport_rateapproved.These attributes enhance user control over the output of the tracking process.
532-547: Newrun_stepmethod approved.This method clears tracks and processes instances for tracking.
742-746: New attributesverbosityandreport_rateapproved.These attributes enhance user control over the output of the tracking process.
1484-1488: New attributesverbosityandreport_rateapproved.These attributes enhance user control over the output of the tracking process.
1014-1015: New parametersprogress_reportingandreport_rateapproved.These parameters enhance user control over the output of the tracking process.
Line range hint
1226-1248:
Updatedadd_cli_parser_argsmethod approved.The method updates the CLI parser to include the new
progress_reportingandreport_rateparameters.
1280-1283: Updatedget_by_name_factory_optionsmethod approved.The method updates the factory options to include the new
progress_reportingandreport_rateparameters.
1484-1489: Updatedtrackmethod approved.The method enhances the tracking process by incorporating Kalman filters for better accuracy.
1707-1707: Modifiedretrackfunction approved.The modification aligns with the removal of the standalone
run_trackerfunction.However, ensure that all function calls to
retrackmatch the new implementation.Verification successful
Modified
retrackfunction approved.The modification aligns with the removal of the standalone
run_trackerfunction. The function calls toretrackmatch the new implementation.
tests/nn/test_inference.py: Verified test case involvingretrack.sleap/nn/tracking.py: Verified function definition and call.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `retrack` match the new implementation. # Test: Search for the function usage. Expect: Only occurrences of the new implementation. rg --type python -A 5 $'retrack'Length of output: 62
Script:
#!/bin/bash # Description: Verify all function calls to `retrack` match the new implementation. # Test: Search for the function usage. Expect: Only occurrences of the new implementation. rg --type py -A 5 $'retrack'Length of output: 1193
142-142: Verify the impact of changing the default value ofsave_shifted_instancestoTrue.This change could affect memory usage and performance.
Verification successful
Verify the impact of changing the default value of
save_shifted_instancestoTrue.This change could affect memory usage and performance.
Verify the impact of changing the default value of
save_shifted_instancestoTrue.The change in the default value of
save_shifted_instancestoTruewill increase memory usage due to the saving of shifted instances between frames. However, it is expected to improve instance matching. This behavior is documented and tested in the codebase.
sleap/nn/tracking.py: Multiple references tosave_shifted_instancesshow its impact on saving shifted instances.sleap/gui/learning/runners.py: Configuration references.tests/nn/test_inference.py: Test cases covering the new default behavior.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of changing the default value of `save_shifted_instances` to `True`. # Test: Search for the usage of `save_shifted_instances` in the codebase. Expect: Analyze its impact on memory and performance. rg --type python -A 5 $'save_shifted_instances'Length of output: 77
Script:
#!/bin/bash # Description: Verify the impact of changing the default value of `save_shifted_instances` to `True`. # Test: Search for the usage of `save_shifted_instances` in the codebase. Expect: Analyze its impact on memory and performance. rg --type py -A 5 $'save_shifted_instances'Length of output: 5528
549-653: Newrun_trackermethod approved.This method includes logic for different verbosity modes, providing visual feedback via a progress bar or JSON-formatted progress updates.
However, ensure that all function calls to
run_trackermatch the new signature.Tools
Ruff
614-614: Use
enumerate()for index variablen_processedinforloop(SIM113)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- sleap/config/pipeline_form.yaml (2 hunks)
- sleap/gui/learning/dialog.py (1 hunks)
- sleap/gui/widgets/video.py (2 hunks)
- sleap/nn/inference.py (12 hunks)
- sleap/nn/tracking.py (24 hunks)
- sleap/util.py (2 hunks)
- tests/nn/test_tracker_components.py (2 hunks)
- tests/nn/test_tracking_integration.py (4 hunks)
Files skipped from review as they are similar to previous changes (5)
- sleap/config/pipeline_form.yaml
- sleap/gui/learning/dialog.py
- sleap/gui/widgets/video.py
- sleap/nn/inference.py
- sleap/util.py
Additional context used
Ruff
sleap/nn/tracking.py
614-614: Use
enumerate()for index variablen_processedinforloop(SIM113)
Additional comments not posted (10)
tests/nn/test_tracking_integration.py (3)
22-22: Verify the updated assertion.The expected number of tracks has been reduced from 27 to 8. Ensure this change is correct and consistent with the updated tracking logic.
44-44: LGTM! Ensure correct initialization oftrackerobject.The change to use
tracker.run_trackerwith verbosity improves encapsulation and maintainability.
51-51: LGTM! Ensure correct initialization oftrackerobject.The change to use
tracker.run_trackerwith verbosity improves encapsulation and maintainability.tests/nn/test_tracker_components.py (2)
18-28: LGTM! Improved flexibility and error handling.The addition of the
img_scaleparameter and the use ofpytest.skip()enhance the function's flexibility and robustness.
40-58: LGTM! Enhanced testing coverage.The inclusion of
img_scaleas a parameter broadens the scope of the tests to include variations in image scaling.sleap/nn/tracking.py (5)
520-531: LGTM! Improved user control and reporting.The addition of
verbosityandreport_rateattributes, along with the enhancedrun_trackermethod, improve user control and reporting capabilities.
532-548: LGTM! New methodrun_stepimproves modularity.The
run_stepmethod enhances modularity by encapsulating the logic for processing a single frame.
549-654: LGTM! Enhancedrun_trackermethod.The
run_trackermethod now includes logic for different verbosity modes, providing better user feedback during the tracking process.Tools
Ruff
614-614: Use
enumerate()for index variablen_processedinforloop(SIM113)
704-709: LGTM! Improved user control and reporting.The addition of
verbosityandreport_rateattributes improves user control and reporting capabilities.
1009-1010: LGTM! Updatedmake_tracker_by_namemethod.The
make_tracker_by_namemethod now includesprogress_reportingandreport_rateparameters, enhancing user control over the tracking process.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- sleap/nn/tracking.py (23 hunks)
- tests/nn/test_tracker_components.py (2 hunks)
- tests/nn/test_tracking_integration.py (5 hunks)
Additional context used
Ruff
sleap/nn/tracking.py
614-614: Use
enumerate()for index variablen_processedinforloop(SIM113)
Additional comments not posted (10)
tests/nn/test_tracking_integration.py (3)
22-22: Verify the change in the expected number of tracks.The assertion change from 27 to 8 indicates a significant reduction in the number of tracks. Verify if this change aligns with the intended behavior and updated tracking logic.
45-45: LGTM! Verify the handling of the verbosity parameter.The change to use
tracker.run_trackerwith an additional verbosity parameter is approved. Ensure that the verbosity parameter is handled correctly within thetrackerclass.
52-52: LGTM! Verify the handling of the verbosity parameter.The change to use
tracker.run_trackerwith an additional verbosity parameter is approved. Ensure that the verbosity parameter is handled correctly within thetrackerclass.tests/nn/test_tracker_components.py (2)
18-27: LGTM! Verify the handling of the newimg_scaleparameter.The changes to rename the function and include the
img_scaleparameter enhance flexibility. Ensure that the new parameter and logic are correctly implemented.
40-40: LGTM! Verify the integration of the newimg_scaleparameter.The changes to include the
img_scaleparameter in the tests enhance coverage. Ensure that the new parameter is correctly integrated into the tests.sleap/nn/tracking.py (5)
141-141: Verify the change in the default value ofsave_shifted_instances.The default value of
save_shifted_instancesis changed fromFalsetoTrue. Verify if this change aligns with the intended functionality and does not adversely impact memory usage and performance.
520-521: LGTM! Verify the implementation of new attributes and methods.The new attributes (
verbosity,report_rate) and methods (run_step,run_tracker) enhance the tracking process. Ensure that they are correctly implemented and integrated.Also applies to: 527-531, 532-548, 549-654
704-709: LGTM! Verify the implementation of new attributes and modifications.The new attributes (
verbosity,report_rate) and modifications enhance the tracking process. Ensure that they are correctly implemented and integrated.Also applies to: 742-747
1477-1482: LGTM! Verify the implementation of new attributes.The new attributes (
verbosity,report_rate) enhance the tracking process. Ensure that they are correctly implemented and integrated.
1699-1699: LGTM! Verify the modification to calltracker.run_tracker.The change to call
tracker.run_trackeraligns with the refactor. Ensure that the modification is correctly implemented.
|
Also fixes a bug introduced by #1875, where the |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/nn/inference.py (13 hunks)
Files skipped from review as they are similar to previous changes (1)
- sleap/nn/inference.py
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1894 +/- ##
===========================================
+ Coverage 73.30% 75.48% +2.17%
===========================================
Files 134 133 -1
Lines 24087 24716 +629
===========================================
+ Hits 17658 18657 +999
+ Misses 6429 6059 -370 ☔ View full report in Codecov by Sentry. |
getzze
force-pushed
the
track-progress
branch
from
98c96dc to
0bb7f9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (14)
tests/nn/test_tracking_integration.py (2)
131-134: Approve use ofos.path.joinfor file path handling.The replacement of string concatenation with
os.path.joinfor file path handling is a good improvement. It enhances the robustness of the code across different operating systems.Consider using an f-string for the filename part to improve readability:
return os.path.join( dir, f"{tracker_name}_{int(scale * 100)}_{matcher_name}_{sim_name}.h5" )
40-40: Incomplete refactoring ofrun_trackerfunction.The
run_trackerfunction is still being used directly in the following test files, indicating that the refactoring might not be complete:
tests/nn/test_tracking_integration.pytests/nn/test_tracker_components.pyPlease review these instances to ensure consistency and alignment with the PR objectives.
Analysis chain
Approve removal of
run_trackerfunction.The removal of the
run_trackerfunction and its replacement with direct calls totracker.run_tracker(frames, verbosity="none")in bothmake_ground_truthandcompare_ground_truthfunctions simplifies the code and aligns with the PR objectives.To ensure this change doesn't affect test coverage or introduce regressions, consider running the full test suite and comparing the results with the previous version.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of run_tracker to ensure complete refactoring rg --type python "run_tracker\(" tests/Length of output: 72
Script:
#!/bin/bash # Correcting the file type to search for other usages of run_tracker to ensure complete refactoring rg --type py "run_tracker\(" tests/Length of output: 320
tests/nn/test_tracker_components.py (1)
18-32: Improved function with better naming and image scaling support.The changes to this function are well-implemented:
- The new name
run_tracker_by_namebetter reflects its functionality.- The addition of the
img_scaleparameter increases flexibility.- The error handling for unsupported
img_scaleis good.Consider using a custom exception instead of
pytest.skipfor better error handling outside of test scenarios. For example:class UnsupportedImageScaleError(Exception): pass # ... if img_scale: if hasattr(t, "candidate_maker") and hasattr(t.candidate_maker, "img_scale"): t.candidate_maker.img_scale = img_scale else: raise UnsupportedImageScaleError("img_scale is not defined for this tracker")This would allow for more flexible error handling in non-test environments.
sleap/config/pipeline_form.yaml (2)
458-463: Consider adding a performance note to the documentationThe help text for
save_shifted_instancesmentions improved instance matching at the cost of more memory usage. It might be beneficial to add a note in the user documentation about the performance implications of this option.Consider adding a brief performance note in the user documentation for this new option. This will help users make informed decisions about whether to enable this feature based on their hardware capabilities and project requirements.
458-463: Potential Impact of Removingtracking.max_tracksThe
tracking.max_tracksparameter has been removed from the configuration file, but it is still referenced in several parts of the codebase. This could lead to issues if these parts of the code expect this parameter to be present. Please review the following files and references to ensure consistency and functionality:
sleap/nn/tracking.py: Multiple references tomax_tracksas a parameter and in tracking logic.sleap/nn/inference.py: Reference tomax_tracksin a function call.sleap/gui/learning/runners.py: References totracking.max_tracksin inference parameter logic.Analysis chain
Removal of
tracking.max_tracksparameterThe
tracking.max_tracksparameter has been removed from the configuration. This change might affect users who were relying on this option to limit the number of tracks.Let's check if there are any references to
max_tracksin the codebase that might need to be updated:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to max_tracks in Python files rg --type python '\bmax_tracks\b' -g '!tests/'Length of output: 79
Script:
#!/bin/bash # Corrected command to search for references to max_tracks in Python files rg -t py '\bmax_tracks\b' -g '!tests/'Length of output: 1659
sleap/nn/tracking.py (5)
35-38: Conditional import for cached_property added.The addition of the conditional import for
cached_propertyimproves compatibility across different Python versions. This is a good practice for maintaining backwards compatibility while leveraging newer features when available.Consider adding a comment explaining why
cached_propertyis preferred over regularpropertywhen available, to provide context for future maintainers.
520-522: New attributes added for progress reporting.The addition of
verbosityandreport_rateattributes to theBaseTrackerclass supports the enhanced progress reporting feature mentioned in the PR objectives. This will allow for more flexible and customizable tracking progress updates.Consider adding docstring comments for these new attributes to explain their purpose and expected values, improving the class's documentation.
532-547: New run_step method added to BaseTracker class.The
run_stepmethod encapsulates the logic for processing a single frame in the tracking process. This improves modularity and reusability of the tracking code. The method handles both image-based and non-image-based tracking scenarios, making it versatile.Consider adding a docstring to the
run_stepmethod to explain its purpose, parameters, and return value. This would improve the method's documentation and make it easier for other developers to understand and use.
742-747: Tracker class updated with new progress reporting attributes.The addition of
verbosityandreport_rateattributes to theTrackerclass ensures consistency with theBaseTrackerclass. The use of a validator for theverbosityattribute is a good practice to ensure only valid values are used.Consider adding a validator for the
report_rateattribute to ensure it's a positive number, preventing potential issues with invalid reporting rates:report_rate: float = attr.ib( default=2.0, validator=attr.validators.instance_of(float) and attr.validators.gt(0) )This addition would improve the robustness of the class by preventing invalid input for the reporting rate.
1477-1482: KalmanTracker class updated with progress reporting attributes.The addition of
verbosityandreport_rateattributes to theKalmanTrackerclass ensures consistency with theBaseTrackerandTrackerclasses. This change maintains a uniform interface for progress reporting across different tracker implementations.For consistency with the
Trackerclass, consider usingattr.ib()for thereport_rateattribute and adding a validator:report_rate: float = attr.ib( default=2.0, validator=attr.validators.instance_of(float) and attr.validators.gt(0) )This change would align the attribute definition style with the
verbosityattribute and add an extra layer of input validation.sleap/nn/inference.py (4)
433-434: Consider using enumerate for better readability.Instead of iterating directly over
examples, consider usingenumerate(examples)to get both the index and the example in each iteration. This can make the code more readable and provide the iteration count if needed.for i, ex in enumerate(examples): ex = process_batch(ex) progress.update(task, advance=len(ex["frame_ind"]))
453-454: Consider using enumerate for better readability.Similar to the previous comment, using
enumerate(examples)here could improve readability and provide the iteration count if needed.for i, ex in enumerate(examples): # Process batch of examples. ex = process_batch(ex)
490-491: Consider using enumerate for consistency.For consistency with the previous loops, consider using
enumerate(examples)here as well, even if the index is not used.for i, ex in enumerate(examples): yield process_batch(ex)
5663-5663: Consider adding error handling for tracker.run_tracker.While the change itself is good, it might be beneficial to add some error handling around the
tracker.run_tracker(frames=frames)call. This could help catch and report any issues that might occur during the tracking process.try: frames = tracker.run_tracker(frames=frames) except Exception as e: print(f"Error occurred during tracking: {e}") # Handle the error appropriately
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- sleap/config/pipeline_form.yaml (2 hunks)
- sleap/gui/learning/dialog.py (1 hunks)
- sleap/gui/widgets/video.py (2 hunks)
- sleap/nn/inference.py (13 hunks)
- sleap/nn/tracking.py (23 hunks)
- sleap/util.py (2 hunks)
- tests/nn/test_tracker_components.py (2 hunks)
- tests/nn/test_tracking_integration.py (5 hunks)
Additional context used
Ruff
sleap/nn/tracking.py
614-614: Use
enumerate()for index variablen_processedinforloop(SIM113)
Additional comments not posted (30)
tests/nn/test_tracking_integration.py (2)
45-45: Approve direct call totracker.run_tracker.The replacement of the
run_trackerfunction call with a direct call totracker.run_tracker(frames, verbosity="none")is consistent with the earlier changes and simplifies the code.
22-22: Verify the change in expected number of tracks.The assertion for the number of tracks has been significantly reduced from 27 to 8. This change might be related to the improvements in tracking functionality mentioned in the PR objectives.
Could you provide more context on why the expected number of tracks has changed so drastically? This change might have implications for other parts of the system that depend on the number of tracks.
sleap/util.py (7)
6-6: LGTM: Good use of postponed annotation evaluation.The addition of
from __future__ import annotationsis a good practice. It enables postponed evaluation of annotations, which can improve performance and reduce circular import issues.
16-16: LGTM: Improved import for type checking.The addition of
TYPE_CHECKINGto the import statement is a good practice. It allows for conditional imports that are only evaluated during type checking, which can optimize runtime performance.
20-24: LGTM: Improved import handling and new feature support.The try-except block for importing
filesensures compatibility with different Python versions. The addition ofrich.progressimport supports the newRateColumnclass functionality.Also applies to: 30-30
36-38: LGTM: Fixed undefined name issue.The conditional import of
Taskfromrich.progressaddresses the issue mentioned in the previous comment about the undefined nameTask. It's now properly imported only during type checking.
40-48: LGTM: New feature for improved progress visualization.The new
RateColumnclass is a great addition that aligns with the PR objective of improving progress visualization. It provides a custom rendering of the progress rate, displaying the speed in FPS.
Line range hint
315-315: LGTM: Improved path handling.The modification to use
files("sleap").joinpath(filename)is a more modern and flexible approach for creating the package file path.
Line range hint
320-321: LGTM: Enhanced config file handling.The additions to the
get_config_filefunction provide more flexibility in config file handling. The new parametersignore_file_not_foundandget_defaults, along with the associated logic, allow for ignoring file not found errors and retrieving default configs. This aligns well with the PR objective of improving default options handling.Also applies to: 332-333, 340-341, 349-350
sleap/config/pipeline_form.yaml (3)
458-463: New option to save shifted instances during trackingThe addition of the
tracking.save_shifted_instancesparameter aligns with the PR objectives of improving tracking functionality. This new option allows users to save flow-shifted instances between frames, which can enhance instance matching at the cost of increased memory usage.This change improves the flexibility of the tracking process and gives users more control over the trade-off between accuracy and memory usage.
458-463: Summary of changes to tracking configurationThe modifications to the tracking configuration in this file align with the PR objectives of improving tracking functionality. The addition of the
save_shifted_instancesoption provides users with more control over the tracking process, allowing for a trade-off between improved instance matching and memory usage.However, the removal of the
max_tracksparameter might affect existing users who relied on this option. It's important to ensure that this change doesn't break any existing functionality and that appropriate alternatives are provided if necessary.These changes enhance the flexibility of the tracking process. However, please consider the following actions:
- Update the user documentation to reflect these changes, especially regarding the new
save_shifted_instancesoption and its performance implications.- Verify that the removal of
max_tracksdoesn't negatively impact existing users or workflows.- If
max_tracksfunctionality is still needed, consider implementing it in a different way or providing clear migration instructions for users.
458-463: Verify the impact of saving shifted instances on memory usageWhile the new
save_shifted_instancesoption can improve instance matching, it's important to ensure that the increased memory usage doesn't cause issues in resource-constrained environments.Let's check if there are any memory-related comments or considerations in the codebase:
sleap/gui/learning/dialog.py (2)
736-736: Approve: Prevent negative count with max functionThe modification to use
max(0, total_count - new_counts)is a good improvement. It ensures thatno_result_countis never negative, which could happen ifnew_countssomehow exceededtotal_count. This change adds robustness to the code and prevents potential issues or confusing output related to negative frame counts.
736-736: Verify similar calculations in the codebaseWhile this change is good, it might be worth checking if there are similar calculations elsewhere in the codebase that could benefit from the same robustness check.
Let's search for similar patterns in the codebase:
sleap/nn/tracking.py (7)
4-9: Imports updated to support new functionality.The new imports (json, sys, time) are added to support the enhanced tracking and reporting features. The removal of the unused
defaultdictimport aligns with the previous review comment and improves code cleanliness.
33-34: New import for progress reporting.The addition of
RateColumnfromsleap.utilsupports the enhanced progress reporting feature in the tracking system.
141-141: Default value for save_shifted_instances changed to True.This change aligns with the PR objectives, which mentioned changing the default value for the
save_shifted_instancesoption to True. This modification will likely improve the default behavior of instance tracking.
527-531: New cached_property for report_period added.The addition of the
report_periodcached_property provides a clean and efficient way to calculate the time interval between progress reports. This improves code readability and potentially performance by caching the calculated value.
1085-1086: Progress reporting parameters added to make_tracker_by_name method.The addition of
progress_reportingandreport_rateparameters to themake_tracker_by_namemethod allows for customization of the progress reporting features when creating a tracker instance. This change provides flexibility in how tracking progress is reported, aligning with the enhanced progress reporting capabilities introduced in this update.
Line range hint
1-1699: Summary of changes and overall assessment.The modifications in this file significantly enhance the progress reporting capabilities of the tracking system, aligning well with the PR objectives. Key improvements include:
- Addition of verbosity modes for flexible progress reporting.
- Implementation of a rich progress bar and JSON output options.
- Consistent updates across different tracker implementations (BaseTracker, Tracker, KalmanTracker).
- New methods for step-by-step tracking with progress updates.
These changes will greatly improve the user experience by providing better feedback during the tracking process. The code is generally well-structured and consistent, with only minor suggestions for improvements in documentation and input validation.
The overall changes are well-implemented and will positively impact the SLEAP application's tracking functionality.
Tools
Ruff
614-614: Use
enumerate()for index variablen_processedinforloop(SIM113)
614-614: False positive from static analysis tool.The static analysis tool (Ruff) suggests using
enumerate()for the index variablen_processedin the for loop. However, this suggestion is not applicable in this context. The variablen_processedis not used as an index for the loop, but rather as a counter of processed frames. The current implementation is correct and more efficient for its purpose.The current implementation is appropriate for the intended functionality.
Tools
Ruff
614-614: Use
enumerate()for index variablen_processedinforloop(SIM113)
sleap/gui/widgets/video.py (3)
819-820: LGTM: Good addition for mouse event handling.The new instance variable
_down_posis a good practice for tracking the initial position of the mouse when a button is pressed. This will allow for more precise control over mouse events, such as distinguishing between clicks and drags.
1044-1045: LGTM: Improved mouse movement detection.The updated logic for detecting mouse movement during a click is more robust. By checking if
_down_posis not None before comparing it with the current position, we prevent potential errors that could occur if_down_poshasn't been set (e.g., if mouseReleaseEvent is somehow triggered without a corresponding mousePressEvent).
Line range hint
1-1185: Overall: Good improvements to mouse event handling.The changes in this file are focused on improving the mouse event handling, particularly in the
mouseReleaseEventmethod. The addition of the_down_posinstance variable and its use in detecting mouse movement during clicks enhance the robustness of the user interaction logic.These modifications should lead to more reliable distinction between clicks and drags, improving the overall user experience without introducing unnecessary complexity.
sleap/nn/inference.py (6)
49-53: LGTM: Conditional import of cached_property.This change adds compatibility for Python versions below 3.8 by conditionally importing
cached_property. This is a good practice for maintaining backwards compatibility.
415-417: Improved ETA calculation.Compiling the loop examples before starting the timer is a good optimization for improving the accuracy of the ETA calculation. This change addresses the PR objective of enhancing the ETA calculation in the GUI progress bar.
5388-5390: Updated LabelsReader initialization.The change in the LabelsReader initialization adds the
example_indicesparameter, which is derived fromframe_list(args.frames). This modification allows for more flexible frame selection during inference, addressing the PR objective of adding functionality to run tracking only on defined frames.
5484-5487: Added progress_reporting to tracker initialization.The addition of the
progress_reportingparameter to the tracker initialization is a good improvement. It allows for consistent progress reporting between the predictor and the tracker.
5655-5660: Improved frame filtering for tracking.This change improves the efficiency of frame filtering when running tracking on an existing prediction file. By converting the
example_indicesto a set, the search operation becomes O(1) instead of O(n), which can significantly speed up the process for large datasets.
5684-5684: Use of os.fspath for consistent path handling.The use of
os.fspath(data_path)is a good practice for ensuring consistent path handling across different operating systems. This change improves the robustness of the code.
getzze
force-pushed
the
track-progress
branch
from
ed8d14b to
854432a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (11 hunks)
- sleap/nn/tracking.py (23 hunks)
Additional context used
Learnings (1)
sleap/nn/inference.py (1)
Learnt from: getzze PR: talmolab/sleap#1894 File: sleap/nn/inference.py:164-167 Timestamp: 2024-09-03T14:11:31.284Z Learning: When deciding between `@cached_property` and `@lru_cache`, prefer `@cached_property` when the goal is to ensure that the cached value is automatically deleted when the instance is deleted, avoiding potential memory leaks.
Ruff
sleap/nn/tracking.py
568-568: Use
enumerate()for index variablen_processedinforloop(SIM113)
Additional comments not posted (13)
sleap/nn/tracking.py (9)
141-141: Approved: Default value forsave_shifted_instanceschanged toTrue.The default value has been appropriately changed from
FalsetoTrue. As mentioned in the PR objectives, this restores access to thesave_shifted_instancessetting in the GUI, which was removed in a previous commit.Saving the shifted instances between elapsed frames uses a bit more memory but leads to better instance matches.
520-521: Approved: Newverbosityandreport_rateattributes added toBaseTracker.The addition of the
verbosityandreport_rateattributes to theBaseTrackerclass aligns with the PR objective of introducing different verbosity modes for progress reporting during tracking.
verbositylikely controls the mode of progress reporting (e.g., "rich", "json", "none").report_ratelikely controls the frequency of progress updates.These attributes will enable more flexible and informative progress reporting during the tracking process.
527-530: Approved: Newreport_periodproperty added toBaseTracker.The addition of the
report_periodproperty to theBaseTrackerclass is a useful enhancement. It provides a convenient way to access the time interval between progress reports, calculated based on thereport_rateattribute.Using the
@cached_propertydecorator ensures that the value is calculated only once and cached for subsequent accesses, which can improve performance by avoiding redundant calculations.
549-597: Approved: New_run_tracker_jsonmethod added toBaseTracker.The addition of the
_run_tracker_jsonmethod to theBaseTrackerclass is a significant enhancement that implements the "json" verbosity mode for progress reporting during tracking. This aligns with the PR objective of introducing different verbosity modes for progress reporting.Key features of the
_run_tracker_jsonmethod:
- Iterates over the frames, calls
run_stepon each frame, and yields the new labeled frame.- Tracks the progress and timing information, including the number of processed frames, total frames, elapsed time, processing rate, and estimated time remaining (ETA).
- Reports the progress in JSON format, which can be easily captured and parsed for programmatic monitoring.
- Uses the
max_lengthargument to limit the length of the deques used for calculating the recent processing rate.This method will greatly enhance the ability to monitor the progress of the tracking process programmatically, especially in scenarios like headless servers or notebooks.
Tools
Ruff
568-568: Use
enumerate()for index variablen_processedinforloop(SIM113)
599-623: Approved: New_run_tracker_richmethod added toBaseTracker.The addition of the
_run_tracker_richmethod to theBaseTrackerclass is another significant enhancement that implements the "rich" verbosity mode for progress reporting during tracking. This aligns with the PR objective of introducing different verbosity modes for progress reporting.Key features of the
_run_tracker_richmethod:
- Uses the
richlibrary to display a visually appealing and informative progress bar during tracking.- Iterates over the frames, calls
run_stepon each frame, and yields the new labeled frame.- Updates the progress bar with the current progress percentage, estimated time remaining (ETA), and processing rate.
- Manually refreshes the progress bar based on the
report_periodto support notebooks.This method will greatly improve the user experience by providing a clear and informative visual representation of the tracking progress, especially when running in interactive environments like notebooks.
625-661: Approved: Updatedrun_trackermethod inBaseTracker.The updates to the
run_trackermethod in theBaseTrackerclass are a significant improvement that incorporates the new verbosity modes for progress reporting during tracking. This aligns with the PR objective of introducing different verbosity modes for progress reporting.Key changes in the updated
run_trackermethod:
- Accepts
frames,verbosity, andfinal_passas arguments.- Checks the
is_validproperty to determine if tracking should be performed.- Uses the
verbosityargument or theverbosityattribute to determine the progress reporting mode.- Delegates the actual tracking and progress reporting to the appropriate
_run_tracker_*method based on the verbosity mode.- Supports running a final pass after tracking if
final_passisTrue.These updates provide flexibility and modularity in the tracking process, allowing users to choose the desired verbosity mode and enabling the integration of different progress reporting mechanisms.
750-754: Approved: Updatedverbosityand addedreport_rateattributes inTracker.The changes to the
verbosityandreport_rateattributes in theTrackerclass are in line with the PR objective of introducing different verbosity modes for progress reporting during tracking.
- The
verbosityattribute now has a default value of"none", meaning that by default, no progress reporting will be displayed during tracking.- The validator for the
verbosityattribute ensures that only valid verbosity modes ("none","rich","json") can be assigned.- The addition of the
report_rateattribute with a default value of2.0allows controlling the frequency of progress updates during tracking.These changes provide flexibility in configuring the progress reporting behavior of the
Trackerclass, enabling users to choose the desired verbosity mode and update frequency.
1228-1233: Verify the data type consistency for thesave_shifted_instancesoption.The default value for the
save_shifted_instancesoption in theTracker.get_by_name_factory_optionsmethod has been changed fromFalseto1. This change aligns with the modification made to thesave_shifted_instancesattribute in theFlowCandidateMakerclass, as mentioned in the PR objectives.However, there seems to be an inconsistency in the data type used for the default value. The option's data type is specified as
int, but using1instead ofTruemight cause confusion.Please ensure that the data type for the
save_shifted_instancesoption is consistently used throughout the codebase. If the intended data type isbool, consider updating the default value toTruefor clarity.
1247-1250: Approved: Updated help text for thekf_init_frame_countoption.The changes to the help text for the
kf_init_frame_countoption in theTracker.get_by_name_factory_optionsmethod provide clearer guidance on the behavior of this option.By explicitly mentioning that setting
kf_init_frame_countto0means no Kalman filters will be used, the updated help text improves the understanding of this option's functionality and makes it easier for users to configure the tracking behavior correctly.sleap/nn/inference.py (4)
49-53: LGTM!The code changes are approved. Conditionally defining
cached_propertybased on the Python version is a good way to maintain compatibility.
164-167: The existing discussion in the comments is still relevant. No need to duplicate the reasoning for using@cached_propertyhere.
371-407: LGTM!The new
_process_batchmethod looks good. It nicely encapsulates the batch processing logic and correctly handles updating the input dictionary with model predictions and adjusting for potential scaling.
Line range hint
408-523: Looks good!The new
_run_batch_jsonand_run_batch_richmethods are a nice addition to provide different progress reporting modes. The implementation looks solid, correctly tracking progress, timing and reporting.Updating
_predict_generatorto use these methods based on theverbositysetting is the right approach.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
sleap/nn/tracking.py (1)
549-597: Useenumerate()for index variablen_processed.The static analysis tool suggests using
enumerate()for the index variablen_processedin theforloop.Consider applying this change to improve code readability:
for n_processed, lf in enumerate(frames, start=1): new_lf = self.run_step(lf) # Track timing and progress elapsed_all = time() - t0_all n_batch += 1 # Report if time() > last_report + self.report_period: # ... (rest of the code remains the same)Tools
Ruff
568-568: Use
enumerate()for index variablen_processedinforloop(SIM113)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/nn/inference.py (11 hunks)
- sleap/nn/tracking.py (23 hunks)
Additional context used
Learnings (1)
sleap/nn/inference.py (1)
Learnt from: getzze PR: talmolab/sleap#1894 File: sleap/nn/inference.py:164-167 Timestamp: 2024-09-03T14:11:31.284Z Learning: When deciding between `@cached_property` and `@lru_cache`, prefer `@cached_property` when the goal is to ensure that the cached value is automatically deleted when the instance is deleted, avoiding potential memory leaks.
Ruff
sleap/nn/tracking.py
568-568: Use
enumerate()for index variablen_processedinforloop(SIM113)
Additional comments not posted (14)
sleap/nn/tracking.py (10)
532-661: New run_tracker method added with enhanced progress reporting.The
run_trackermethod is a significant addition that implements comprehensive progress reporting for the tracking process. It supports different verbosity modes ("rich", "json", and default) as mentioned in the PR objectives. This enhancement will greatly improve user experience by providing better feedback during the tracking process.Key improvements:
- Support for rich progress bar display
- JSON output option for programmatic monitoring
- Flexible reporting rate
Consider the following improvements:
- Extract the progress reporting logic for each verbosity mode into separate methods to improve readability and maintainability.
- Add error handling for potential exceptions during the tracking process.
- Consider using a context manager for the "json" verbosity mode to ensure proper resource management, similar to the "rich" mode.
Tools
Ruff
568-568: Use
enumerate()for index variablen_processedinforloop(SIM113)
599-623: LGTM!The
_run_tracker_richmethod effectively utilizes therichlibrary to display an informative and visually appealing progress bar for the "rich" verbosity mode. The code is well-structured and follows best practices.
750-754: LGTM!The addition of the
verbosityandreport_rateattributes to theTrackerclass aligns with the enhanced progress reporting feature. The validation of theverbosityattribute ensures that only valid verbosity modes are accepted, and the default value ofreport_rateis reasonable.
1305-1319: LGTM!The changes to the
add_cli_parser_argsmethod are necessary to support the new "tracker" option without a default value, allowing for the detection of malformed command-line arguments. The addition of the other command-line arguments with their default values is consistent with the existing code.
Line range hint
1017-1094: LGTM!The changes to the
make_tracker_by_namemethod are consistent with the enhanced progress reporting feature. The newprogress_reportingargument allows for specifying the desired verbosity mode when creating aTrackerinstance, and theverbosityandreport_rateattributes are set accordingly. The changes are well-integrated with the existing code.
1228-1233: LGTM!The change in the default value of the
save_shifted_instancesoption fromFalsetoTruealigns with the PR objective of restoring access to this setting in the GUI. The updated help text accurately reflects the change in default value and provides clarity on the behavior of the option. The change is consistent with the overall goals of the PR.
1247-1250: LGTM!The updated help text for the
kf_init_frame_countoption provides clearer guidance on the behavior of the option. It explicitly states that a value of 0 indicates that no Kalman filters will be used. This clarification improves the usability and understanding of the option. The change is minor but beneficial.
1271-1274: LGTM!The updated help text for the
oks_score_weightingoption provides a clearer explanation of the behavior when the value is set to 0 or 1. It specifies that a value of 0 (default) means only the distance between the reference and query keypoint is used to compute the similarity, while a value of 1 means each distance is weighted by the prediction scores of the reference and query keypoint. This clarification enhances the understanding and usability of the option. The change is minor but valuable.
1282-1285: LGTM!The updated help text for the
oks_normalizationoption provides a detailed explanation of the behavior for different values. It clarifies that a value of "all" means the similarity score is normalized by the number of reference points, "ref" means the score is normalized by the number of visible reference points, and "union" means the score is normalized by the number of points both visible in query and reference instance. This detailed explanation improves the understanding and usability of the option. The change is valuable for users to make informed decisions when configuring the tracker.
1485-1489: LGTM!The addition of the
verbosityand `report_sleap/nn/inference.py (4)
49-53: LGTM!The code adds compatibility for Python versions below 3.8 by conditionally defining
cached_property. This is a good practice to ensure backward compatibility.
164-167: Good optimization based on learnings!The change from
@propertyto@cached_propertyforreport_periodis a good optimization. As mentioned in the learnings:When deciding between
@cached_propertyand@lru_cache, prefer@cached_propertywhen the goal is to ensure that the cached value is automatically deleted when the instance is deleted, avoiding potential memory leaks.This ensures that the cached value is automatically deleted when the instance is deleted, avoiding potential memory leaks.
371-407: Improved modularity and clarity!The new
_process_batchmethod encapsulates the logic for running inference on a batch and updating the input dictionary with predictions. This improves modularity and clarity by extracting the inline processing logic into a dedicated method. The method handles important postprocessing steps such as:
- Skipping inference if the model is not loaded
- Running inference on the current batch
- Adding model outputs to the input data example
- Converting
video_indandframe_indto numpy arrays if they are TensorFlow tensors- Adjusting
instance_peaksbased on potentialSizeMatcherscalingThe code is well-structured and follows good practices.
508-523: Improved ETA calculation and flexible progress reporting!The changes introduce several improvements to the inference loop:
Compiling loop examples before starting the timer improves the ETA calculation by providing a more accurate estimate of the total number of examples. This enhances the user experience by providing a more reliable progress estimate.
The optional progress reporting based on the
verbosityattribute allows for flexibility in how progress is displayed:
- The
"rich"option displays an informative and visually appealing progress bar using the_run_batch_richmethod.- The
"json"option outputs progress information as JSON using the_run_batch_jsonmethod, which can be useful for programmatic monitoring or logging.- The
"none"option allows for silent execution without any progress reporting, suitable for scenarios where progress updates are not needed.The code is well-organized and uses appropriate methods for different verbosity levels, making it easy to understand and maintain.
getzze
force-pushed
the
track-progress
branch
from
854432a to
25cd34b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- sleap/config/pipeline_form.yaml (2 hunks)
- sleap/gui/learning/dialog.py (1 hunks)
- sleap/gui/widgets/video.py (2 hunks)
- sleap/nn/inference.py (11 hunks)
- sleap/nn/tracking.py (23 hunks)
- sleap/util.py (2 hunks)
- tests/nn/test_tracker_components.py (2 hunks)
- tests/nn/test_tracking_integration.py (5 hunks)
Files skipped from review as they are similar to previous changes (4)
- sleap/config/pipeline_form.yaml
- sleap/gui/learning/dialog.py
- sleap/gui/widgets/video.py
- sleap/util.py
Additional context used
Learnings (1)
sleap/nn/inference.py (1)
Learnt from: getzze PR: talmolab/sleap#1894 File: sleap/nn/inference.py:164-167 Timestamp: 2024-09-03T14:11:31.284Z Learning: When deciding between `@cached_property` and `@lru_cache`, prefer `@cached_property` when the goal is to ensure that the cached value is automatically deleted when the instance is deleted, avoiding potential memory leaks.
Ruff
sleap/nn/tracking.py
568-568: Use
enumerate()for index variablen_processedinforloop(SIM113)
Additional comments not posted (22)
tests/nn/test_tracking_integration.py (4)
22-22: Verify the change in the expected number of tracks.The assertion in the
test_simple_trackerfunction has been updated to expect 8 tracks instead of 27. This change suggests a potential adjustment in the underlying tracking logic or the data being processed.Please confirm that this change aligns with the intended behavior and provide a brief explanation for the adjustment in the expected number of tracks.
45-45: Verify the impact of removing therun_trackerfunction.The
run_trackerfunction has been removed, and its functionality has been replaced by a direct call totracker.run_tracker(frames, verbosity="none")in themake_ground_truthfunction. While this change simplifies the code, it's important to ensure that it doesn't introduce any unintended side effects or break existing functionality.Please confirm that the removal of the
run_trackerfunction and the direct utilization of the tracker's method have been thoroughly tested and do not negatively impact the overall behavior of the code.
52-52: Verify the impact of removing therun_trackerfunction in thecompare_ground_truthfunction.Similar to the change in the
make_ground_truthfunction, therun_trackerfunction has been removed, and its functionality has been replaced by a direct call totracker.run_tracker(frames, verbosity="none")in thecompare_ground_truthfunction. While this change simplifies the code, it's important to ensure that it doesn't introduce any unintended side effects or break existing functionality.Please confirm that the removal of the
run_trackerfunction and the direct utilization of the tracker's method in thecompare_ground_truthfunction have been thoroughly tested and do not negatively impact the overall behavior of the code.
131-133: LGTM!The modification in the
make_filenamefunction, where the string concatenation for the file path has been replaced withos.path.join, improves the robustness of file path handling across different operating systems. This change enhances the portability and maintainability of the code.tests/nn/test_tracker_components.py (3)
18-32: LGTM!The new
run_tracker_by_namefunction is well-structured and handles theimg_scaleparameter appropriately. Skipping the tracking process whenimg_scaleis not supported is a good practice to prevent unnecessary execution. The assertion at the end ensures that the tracking process produces the expected number of frames.
40-40: Also applies to: 47-47, 53-58
Line range hint
77-84:sleap/nn/tracking.py (11)
625-661: New run_tracker method added with enhanced progress reporting.The
run_trackermethod is a significant addition that implements comprehensive progress reporting for the tracking process. It supports different verbosity modes ("rich", "json", and default) as mentioned in the PR objectives. This enhancement will greatly improve user experience by providing better feedback during the tracking process.
599-623: LGTM!The
_run_tracker_richmethod is well-implemented and provides a rich progress bar for tracking progress. It uses a context manager to ensure proper resource management.
532-547: LGTM!The
run_stepmethod is correctly implemented and clears the tracks before running the tracking step.
527-530: LGTM!The
report_periodcached property is correctly implemented and uses thereport_rateattribute to calculate the report period.
750-754: LGTM!The new
verbosityandreport_rateattributes are correctly added to theTrackerclass and will be used for progress reporting during tracking.
1017-1018: LGTM!The
make_tracker_by_nameclass method correctly handles the newprogress_reportingandreport_ratearguments and passes them to theTrackerconstructor.
1228-1233: LGTM!The
get_by_name_factory_optionsclass method correctly updates the default value ofsave_shifted_instancesto 1. This change aligns with the PR objective of restoring access to this setting in the GUI.
1305-1312: LGTM!The
add_cli_parser_argsclass method correctly handles the case when the default value is defined for "tracking.tracker" by not passing the default value to the argument parser. This change prevents the detection of malformed command line arguments.
1485-1489: LGTM!The new
verbosityandreport_rateattributes are correctly added to theKalmanTrackerclass and will be used for progress reporting during tracking.
1707-1707: LGTM!The
run_trackermethod of theKalmanTrackerclass correctly passes theframesargument to theinit_tracker.run_trackermethod.
568-568: Skip the Ruff suggestion.The suggestion to use
enumerate()for index variablen_processedis not applicable in this case becausen_processedis not used as an index variable in theforloop. It is a counter variable that is incremented in each iteration of the loop. Usingenumerate()would not provide any benefit here.Tools
Ruff
568-568: Use
enumerate()for index variablen_processedinforloop(SIM113)
sleap/nn/inference.py (4)
164-167: LGTM!The code changes are approved. Using
@cached_propertyis the right choice here to avoid potential memory leaks.
371-407: Looks good!The new
_process_batchfunction correctly handles running inference on a batch and postprocessing the results. The logic for skipping inference when the model is not loaded, running inference, updating the input dictionary, and adjusting for SizeMatcher scaling is implemented properly.
408-455: LGTM!The new
_run_batch_jsonfunction correctly processes batches of examples and reports the progress in JSON format. The logic for tracking timing and progress, computing the recent rate and ETA, and printing the JSON progress information at the specified report period is implemented properly.
456-486: Looks good!The new
_run_batch_richfunction correctly processes batches of examples and reports the progress using the rich progress bar. The setup of the rich progress bar with custom configuration, updating the progress for each batch, and manually refreshing the progress bar to support notebooks is implemented properly.
|
It's ready for review! |
getzze
force-pushed
the
track-progress
branch
from
b78b51b to
10799b9
Compare
|
Thanks for the updates @getzze! Just keep us posted on when it's ready for review again. |
|
It's ready to review! |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (3)
sleap/util.py (1)
40-48: LGTM: Well-implementedRateColumnclass for progress tracking.The
RateColumnclass is a good addition for displaying the progress rate in FPS. It handles edge cases well and provides a clear, formatted output.Consider adding a brief comment explaining the meaning of FPS in this context, as it might not be immediately clear to all developers that it refers to "Frames Per Second" in this tracking scenario. For example:
return rich.progress.Text(f"{speed:.1f} FPS", style="progress.data.speed") # FPS: Frames Per Secondsleap/config/pipeline_form.yaml (1)
458-463: LGTM! Consider a minor improvement in the help text.The addition of the
tracking.save_shifted_instancesoption is well-placed and aligns with the PR objectives. The configuration is clear and the default value is set correctly.Consider slightly rewording the help text for clarity:
- help: 'Save the flow-shifted instances between elapsed frames. It improves - instance matching at the cost of using a bit more of memory.' + help: 'Save the flow-shifted instances between elapsed frames. This improves + instance matching at the cost of using more memory.'This minor change improves readability and precision in describing the memory impact.
sleap/nn/inference.py (1)
51-51: Typographical error in commentThere's a typo in the comment on line 51: "define" should be "defined".
Apply this diff to correct the typo:
-else: # cached_property is define only for python >=3.8 +else: # cached_property is defined only for python >=3.8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- sleap/config/pipeline_form.yaml (1 hunks)
- sleap/nn/inference.py (10 hunks)
- sleap/nn/tracking.py (13 hunks)
- sleap/util.py (1 hunks)
- tests/nn/test_tracker_components.py (3 hunks)
- tests/nn/test_tracking_integration.py (4 hunks)
🧰 Additional context used
📓 Learnings (1)
sleap/nn/inference.py (1)
<retrieved_learning>
Learnt from: getzze
PR: #1894
File: sleap/nn/inference.py:164-167
Timestamp: 2024-09-03T14:11:31.284Z
Learning: When deciding between@cached_propertyand@lru_cache, prefer@cached_propertywhen the goal is to ensure that the cached value is automatically deleted when the instance is deleted, avoiding potential memory leaks.
</retrieved_learning>
🪛 Ruff
sleap/nn/tracking.py
571-571: Use
enumerate()for index variablen_processedinforloop(SIM113)
sleap/util.py
8-8:
base64imported but unusedRemove unused import:
base64(F401)
31-31:
PIL.Imageimported but unusedRemove unused import:
PIL.Image(F401)
🔇 Additional comments (14)
tests/nn/test_tracking_integration.py (4)
5-5: LGTM: Import Path from pathlibThe addition of
from pathlib import Pathis a good change. It aligns with the project's move towards usingpathlibfor cross-platform path handling.
46-46: LGTM: Simplified tracker executionThe direct use of
tracker.run_tracker(frames, verbosity="none")in bothmake_ground_truthandcompare_ground_truthfunctions is a good simplification. It removes the unnecessary intermediaterun_trackerfunction, making the code more straightforward.Also applies to: 53-53
134-136: LGTM: Improved path handlingThe use of
Path.joinpathfor constructing the file path is an excellent change. It improves cross-platform compatibility and follows the project's move towards usingpathlibfor path handling.
23-23: Verify the change in expected number of tracksThe assertion for the number of tracks has been significantly reduced from 27 to 8. This change might reflect improvements in the tracking algorithm or changes in its configuration.
Please confirm:
- Is this change intentional?
- What improvements or changes in the tracking algorithm led to this reduction?
- Are there any implications for the overall tracking performance?
sleap/util.py (2)
29-29: LGTM: New imports for progress tracking and type hinting.The addition of
rich.progressimport and the conditional import ofTaskfor type hinting are appropriate for the newRateColumnclass and improve type checking without affecting runtime performance.Also applies to: 36-37
1-49: Summary: Enhancements align well with PR objectives.The changes in this file, particularly the addition of the
RateColumnclass, contribute significantly to the PR's goal of improving tracking functionality and progress display in the SLEAP application. These modifications lay the groundwork for implementing the progress bar during tracking, as outlined in the PR objectives.The new imports and class definition are well-structured and follow best practices. With the suggested minor improvements (removing unused imports and adding a clarifying comment), this file will be in excellent shape to support the enhanced tracking features.
🧰 Tools
🪛 Ruff
8-8:
base64imported but unusedRemove unused import:
base64(F401)
31-31:
PIL.Imageimported but unusedRemove unused import:
PIL.Image(F401)
tests/nn/test_tracker_components.py (2)
18-32: Functionrun_tracker_by_nameimplemented correctlyThe new
run_tracker_by_namefunction correctly replacestracker_by_nameand appropriately handles theimg_scaleparameter.
50-61: Parameters added totest_tracker_by_nameappropriatelyThe addition of
img_scaleandcountparameters totest_tracker_by_nameis appropriate and correctly integrated into the test function.sleap/nn/tracking.py (5)
4-8: LGTM on added imports for progress reporting functionalityThe newly added imports are appropriate and necessary for the implementation of progress reporting features.
12-13: Importingrich.progressenhances progress bar capabilitiesThe addition of
import rich.progressis essential for the rich progress bar implementation in the tracking process.
602-627: New method_run_tracker_richadds rich progress bar displayThe
_run_tracker_richmethod introduces a rich progress bar during tracking, significantly improving user experience by providing visual feedback.
552-601: New method_run_tracker_jsonprovides JSON-based progress reportingThe implementation of the
_run_tracker_jsonmethod effectively adds JSON-based progress reporting, enabling programmatic monitoring of the tracking process.🧰 Tools
🪛 Ruff
571-571: Use
enumerate()for index variablen_processedinforloop(SIM113)
628-665: Updates torun_trackermethod to incorporate verbosity optionsThe
run_trackermethod now supports different verbosity levels, allowing for flexible progress reporting tailored to various use cases.sleap/nn/inference.py (1)
508-509: Confirm thatdata_providersupportslen()In the line
n_total = len(data_provider), ensure thatdata_providerimplements the__len__method. If not, this will raise aTypeError. Verify thatdata_providersupports the length operation.
|
Hi @getzze , I rebased you branch off of our updated Thanks, Divya |
|
Hi @gitttt-1234, Thanks |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
sleap/nn/tracking.py (1)
37-40: Typographical error in comment on line 39.The comment on line 39 contains a typo: "define" should be "defined".
Apply this diff to correct the typo:
-else: # cached_property is define only for python >=3.8 +else: # cached_property is defined only for python >=3.8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- sleap/nn/tracking.py (14 hunks)
- tests/nn/test_tracker_components.py (3 hunks)
- tests/nn/test_tracking_integration.py (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/nn/test_tracker_components.py
- tests/nn/test_tracking_integration.py
🧰 Additional context used
🪛 Ruff
sleap/nn/tracking.py
572-572: Use
enumerate()for index variablen_processedinforloop(SIM113)
🔇 Additional comments (14)
sleap/nn/tracking.py (14)
4-9: Imports are appropriately added for new functionality.The imports of
json,sys,deque,time, and typing annotations are necessary for the added methods and enhancements.
12-13: Necessary importsnumpyandrich.progressadded.These imports are required for numerical computations and enhanced progress reporting.
35-36: Import ofRateColumnfor progress bar display.The
RateColumnis used in the rich progress bar to display processing rates.
523-525: Duplicate comment: Missing default values and validators forverbosityandreport_rate.
530-534: Duplicate comment: Potential division by zero inreport_perioddue toreport_ratebeing zero.
553-602: The_run_tracker_jsonmethod is correctly implemented for JSON progress reporting.The method effectively handles progress calculations and outputs, providing useful feedback during tracking.
🧰 Tools
🪛 Ruff
572-572: Use
enumerate()for index variablen_processedinforloop(SIM113)
603-628: The_run_tracker_richmethod utilizesrichlibrary effectively for progress display.The implementation is appropriate and follows best practices for progress bar integration.
630-666:run_trackermethod incorporates verbosity options and handles execution flow correctly.The method correctly delegates to the appropriate progress reporting method based on the
verbositysetting.
755-760: Addedverbosityandreport_ratewith validation inTrackerclass.Including validators and default values ensures robust usage of these attributes.
1029-1030: Duplicate comment: Inconsistent defaultverbosityvalues across classes.
1105-1106: Passingverbosityandreport_rateto the tracker instance.This ensures that the configured verbosity and report rate are correctly applied.
1497-1502: Addedverbosityandreport_ratewith validation inKalmanTrackerclass.Consistent implementation with the
Trackerclass.
1566-1566: Thetrackmethod inKalmanTrackercorrectly implements the abstract base class signature.Ensures compatibility with the
BaseTracker.
1720-1720: Updatedretrackfunction to userun_trackermethod.This change aligns the main function with the new tracking workflow.
change default save_shifted_instances
getzze
force-pushed
the
track-progress
branch
from
ca956ae to
3b7a75b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
tests/nn/test_tracking_integration.py (1)
Line range hint
89-193: Consider restructuring test file organization.The file mixes different types of test code:
- Unit tests (
test_simple_tracker,test_simplemax_tracker)- Integration test helpers (
make_ground_truth,compare_ground_truth)- Development/debugging code (
mainfunction)Consider:
- Moving the development/debugging code to a separate script
- Creating a proper test fixture for the shared test setup
- Using pytest's parametrize feature for testing different tracker configurations
tests/nn/test_tracker_components.py (3)
17-17: Add type hints and docstring for better code documentation.Consider adding type hints for the
framesparameter and a docstring explaining the function's purpose and parameters.-def run_tracker_by_name(frames=None, img_scale: float = 0, **kwargs): +def run_tracker_by_name(frames: Optional[List[Frame]] = None, img_scale: float = 0, **kwargs) -> List[Frame]: + """Run tracker with specified parameters and return tracked frames. + + Args: + frames: List of frames to track. If None, empty list is used. + img_scale: Image scaling factor. If non-zero, updates tracker's candidate_maker. + **kwargs: Additional arguments passed to Tracker.make_tracker_by_name. + + Returns: + List of tracked frames. + """
42-42: Add comment explaining img_scale test values.Consider adding a comment explaining the significance of each img_scale test value for better test documentation.
+# Test different scaling scenarios: +# - 0: Default behavior (no scaling) +# - 1: Original size +# - 0.25: Quarter size downscaling @pytest.mark.parametrize("img_scale", [0, 1, 0.25])
Line range hint
79-87: Consider adding img_scale parameter tests for consistency.For consistency with
test_tracker_by_name, consider parameterizing this test with differentimg_scalevalues to ensure OKS tracking works correctly with image scaling.@pytest.mark.parametrize("tracker", ["simple", "flow", "simplemaxtracks", "flowmaxtracks"]) @pytest.mark.parametrize("oks_score_weighting", ["True", "False"]) @pytest.mark.parametrize("oks_normalization", ["all", "ref", "union"]) +@pytest.mark.parametrize("img_scale", [0, 1, 0.25]) def test_oks_tracker_by_name( centered_pair_predictions_sorted, tracker, oks_score_weighting, oks_normalization, + img_scale, ): # This is slow, so limit to 5 time points frames = centered_pair_predictions_sorted[:5] run_tracker_by_name( frames=frames, tracker=tracker, similarity="object_keypoint", matching="greedy", oks_score_weighting=oks_score_weighting, oks_normalization=oks_normalization, max_tracks=2, + img_scale=img_scale, )sleap/util.py (2)
18-21: Consider tracking the TODO comment with an issue.The conditional import handling is well-implemented, but the TODO comment about upgrading to
importlib.resourcesshould be tracked.Would you like me to create a GitHub issue to track the upgrade of
importlib_resourcestoimportlib.resourcesonce Python 3.9+ becomes the minimum supported version?
37-45: Consider enhancing method documentation.The
RateColumnimplementation is clean and well-structured, but therendermethod could benefit from more detailed documentation.Add parameter and return type documentation to the render method:
def render(self, task: Task) -> rich.progress.Text: - """Show progress rate.""" + """Show progress rate. + + Args: + task: The progress task containing speed information. + + Returns: + Text object containing the formatted speed in FPS. + """ speed = task.speedsleap/config/pipeline_form.yaml (1)
458-463: Consider documenting memory usage implications.The help text for
tracking.save_shifted_instancesmentions memory impact but doesn't specify the expected memory overhead. Consider adding more specific guidance about memory requirements to help users make informed decisions.help: 'Save the flow-shifted instances between elapsed frames. It improves - instance matching at the cost of using a bit more of memory.' + instance matching at the cost of additional memory usage (approximately X MB per frame).'sleap/nn/tracking.py (1)
Line range hint
1098-1101: Handle potential absence of__name__attribute inget_namemethodIn the
get_namemethod, accessing__name__onself.similarity_functionmay raise anAttributeErrorif it is afunctools.partialobject (e.g., when usingnormalized_instance_similaritywith partial arguments), sincepartialobjects do not have a__name__attribute. To ensure robustness, consider usingself.similarity_function.func.__name__orgetattrto safely retrieve the function's name.Apply this diff to fix the issue:
def get_name(self): tracker_name = self.candidate_maker.__class__.__name__ - similarity_name = self.similarity_function.__name__ + similarity_name = getattr( + self.similarity_function, 'func', self.similarity_function + ).__name__ match_name = self.matching_function.__name__ return f"{tracker_name}.{similarity_name}.{match_name}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- sleap/config/pipeline_form.yaml (1 hunks)
- sleap/gui/learning/dialog.py (1 hunks)
- sleap/gui/widgets/video.py (2 hunks)
- sleap/nn/inference.py (10 hunks)
- sleap/nn/tracking.py (12 hunks)
- sleap/util.py (1 hunks)
- tests/nn/test_tracker_components.py (3 hunks)
- tests/nn/test_tracking_integration.py (4 hunks)
🧰 Additional context used
🪛 Ruff
sleap/nn/tracking.py
582-582: Use
enumerate()for index variablen_processedinforloop(SIM113)
🔇 Additional comments (11)
tests/nn/test_tracking_integration.py (4)
4-4: LGTM! Import changes follow best practices.The changes align with:
- Moving from
os.pathtopathlibfor cross-platform compatibility- Making imports more explicit by directly importing
LabelsAlso applies to: 9-9
45-45: LGTM! Progress reporting enhancement.The addition of the
verbosity="none"parameter aligns with the PR's objective to improve progress reporting in the tracking functionality.Also applies to: 52-52
133-135: LGTM! Addressed previous review feedback.The path handling has been correctly updated to use
Path.joinpathas requested in the previous review, improving cross-platform compatibility.
22-22: Verify the change in expected track count.The assertion for the number of tracks has been significantly reduced from 27 to 8. While this might be correct due to the new selective frame tracking feature, please verify that this change is intentional and document the reason for the change.
✅ Verification successful
From analyzing the test files and git history, I can now generate a final response:
Change in track count is intentional and aligns with implementation
The change from 27 to 8 tracks in
test_simple_trackeris correct and intentional based on the following evidence:
- The commit history (2b26078) shows this was an explicit change made alongside "add tracking progress reporting"
- The test file contains multiple tracking-related tests that demonstrate consistent behavior:
test_simple_tracker: 8 tracks (basic tracking)test_simplemax_tracker: 2 tracks (with max tracks constraint)- The test suite includes comprehensive tracking component tests that validate track count behavior under different scenarios:
- Tests verify track pruning
- Tests ensure max track constraints are enforced
- Tests validate tracking window behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if this change is documented or discussed in related PRs/issues # Test: Search for discussions about track count changes rg -l "track.*count|number.*track" .github/Length of output: 45
Script:
#!/bin/bash # Let's check the test file and surrounding context to understand the change better cat tests/nn/test_tracking_integration.py # Also check for any related test files that might provide context fd test_tracking .Length of output: 7223
Script:
#!/bin/bash # Look for commit messages that might explain this change git log -p tests/nn/test_tracking_integration.pyLength of output: 13727
Script:
#!/bin/bash # Search for documentation about tracking functionality fd README.md --exec cat {} \; fd CHANGELOG.md --exec cat {} \;Length of output: 629
Script:
#!/bin/bash # Look for tracking-related configuration or constants that might define this behavior rg -A 3 "track" tests/nn/Length of output: 61924
sleap/util.py (3)
6-6: LGTM: Good use of postponed annotations evaluation.The addition of
from __future__ import annotationsis a good practice that enables postponed evaluation of type annotations, helping to avoid circular imports and reduce memory usage.
33-34: LGTM: Well-structured type checking imports.Good use of
TYPE_CHECKINGto isolate type hints and prevent circular imports.
37-45: Verify RateColumn integration with progress tracking system.Let's ensure the RateColumn is properly integrated with the progress tracking system across the codebase.
✅ Verification successful
Based on the results, I can now generate the final response:
RateColumn is properly integrated with progress tracking system
The verification shows that RateColumn is correctly integrated and used in the codebase:
- Imported and used in
sleap/nn/inference.pyfor prediction progress tracking- Imported and used in
sleap/nn/tracking.pyfor tracking progress- Both implementations properly integrate RateColumn with rich.progress.Progress alongside standard columns like BarColumn and TimeRemainingColumn
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of RateColumn in progress bars # Expected: Find instances where RateColumn is used in progress tracking # Search for RateColumn usage echo "Searching for RateColumn usage in progress bars..." rg -l "RateColumn" | while read -r file; do echo "=== $file ===" rg -A 5 "RateColumn" "$file" done # Search for rich.progress.Progress usage to verify integration echo -e "\nSearching for Progress instantiation..." rg -A 5 "Progress\("Length of output: 2441
sleap/config/pipeline_form.yaml (1)
458-463: Verify the impact of save_shifted_instances on tracking accuracy.While setting
save_shifted_instancesto false by default is good for memory conservation, we should verify that this doesn't significantly impact tracking accuracy in common use cases.✅ Verification successful
Let's examine the test file to understand how
save_shifted_instancesis used.
Let's check the implementation of the flow tracker to understand how
save_shifted_instancesaffects tracking.
Let's check how the shifted instances are used in the tracking process to understand their impact.
Default value for save_shifted_instances is appropriate
Based on the implementation analysis, setting
save_shifted_instancesto false by default is the correct choice:
- When enabled, it caches flow-shifted instances between frames to avoid recomputing them, trading memory for speed
- The feature is purely an optimization that doesn't affect tracking accuracy - it just reuses previously computed optical flow results
- Tests show it's used successfully with both settings, and the default false value prioritizes memory efficiency
- Users can easily enable it if they need the performance optimization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases or benchmarks that compare tracking accuracy with and without shifted instances rg -l "save_shifted_instances" tests/Length of output: 64
Script:
#!/bin/bash # Check the context of save_shifted_instances usage in the test file rg -B 3 -A 3 "save_shifted_instances" tests/nn/test_inference.pyLength of output: 852
Script:
#!/bin/bash # Search for the flow tracker implementation and its usage of save_shifted_instances ast-grep --pattern 'class FlowTracker' -A 50 rg -B 3 -A 3 "save_shifted_instances.*=.*" --type pyLength of output: 2696
Script:
#!/bin/bash # Search for usage of shifted_instances in the tracking implementation rg -B 5 -A 5 "shifted_instances" sleap/nn/tracking.pyLength of output: 8529
sleap/gui/learning/dialog.py (1)
736-736: LGTM! Important fix to prevent negative result counts.The change ensures that
no_result_countcannot be negative by usingmax(0, total_count - new_counts). This is a valuable fix as negative counts would be illogical when reporting inference results to users, potentially causing confusion. The modification maintains data integrity in the GUI's progress reporting.sleap/gui/widgets/video.py (2)
819-820: LGTM: Good fix for the GUI crash issue.Initializing
_down_postoNonein the constructor prevents the GUI from crashing whenmouseReleaseEventis called beforemousePressEvent. This addresses the bug mentioned in the PR objectives.
1044-1044: LGTM: Clean implementation of mouse movement detection.The added check
has_moved = self._down_pos is not None and event.pos() != self._down_poscorrectly determines if the mouse moved during a click, which is essential for differentiating between click and drag actions.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
sleap/nn/tracking.py (1)
538-544: Enhance report_period validation and loggingThe warning message could be more informative, and the fallback value should be documented.
Apply this diff to improve the implementation:
@cached_property def report_period(self) -> float: """Time between progress reports in seconds.""" if self.report_rate <= 0: - logger.warning("report_rate must be positive, fallback to 1") + logger.warning( + f"Invalid report_rate ({self.report_rate}), must be positive. " + "Falling back to default rate of 1 Hz" + ) return 1.0 return 1.0 / self.report_ratesleap/nn/inference.py (1)
Line range hint
4650-4654: Simplify conditional assignment using a ternary operatorYou can simplify the
if-elseblock by using a ternary operator for conciseness.Apply this diff to simplify the conditional assignment:
- if isinstance(ex, dict): - img = ex["image"] - - else: - img = ex + img = ex["image"] if isinstance(ex, dict) else ex
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- sleap/nn/inference.py (14 hunks)
- sleap/nn/tracking.py (12 hunks)
🧰 Additional context used
🪛 Ruff
sleap/nn/inference.py
4650-4654: Use ternary operator
img = ex["image"] if isinstance(ex, dict) else exinstead ofif-else-blockReplace
if-else-block withimg = ex["image"] if isinstance(ex, dict) else ex(SIM108)
sleap/nn/tracking.py
583-583: Use
enumerate()for index variablen_processedinforloop(SIM113)
🔇 Additional comments (3)
sleap/nn/tracking.py (2)
614-639: LGTM: Well-implemented rich progress barThe implementation of the rich progress bar with manual refresh handling for notebooks is well thought out and aligns with the PR objectives for improved progress reporting.
1322-1336: LGTM: Well-implemented CLI argument handlingThe special handling of the 'tracker' argument without a default value is a good solution for detecting malformed command lines, while maintaining defaults for other arguments.
sleap/nn/inference.py (1)
396-399: Verify existence of keys before accessing dictionary itemsWhen accessing
ex["video_ind"]andex["frame_ind"], ensure that these keys exist in theexdictionary to prevent potentialKeyError. Consider usingex.get("video_ind")andex.get("frame_ind")with default values or add checks before accessing.
|
Hi @gitttt-1234, |
Description
Several things were buggy when running
tracking-onlyfrom GUI orsleap-trackfrom CLI with only tracking:Nonevalue, which is boolean-False, so it could give the opposite for True-by-default options (there are none so far, fortunately).save_shifted_instancesto True and give back access to setting it from the GUI (removed in Limit max tracks via track-local queues #1447, I don't know why)data_pathvariable was overwritten.Types of changes
Does this address any currently open issues?
None
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
Summary by CodeRabbit
New Features
movenet-lightning,movenet-thunder, andtracking-only.Bug Fixes
Refactor
Trackerclass, removing standalone functions for better encapsulation.Tests