Fix input scaling in centered-instance model #2054
Conversation
WalkthroughThe pull request introduces modifications across three key files in the SLEAP neural network pipeline: Changes
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2054 +/- ##
===========================================
+ Coverage 75.43% 76.12% +0.69%
===========================================
Files 134 134
Lines 24749 24780 +31
===========================================
+ Hits 18670 18865 +195
+ Misses 6079 5915 -164 ☔ View full report in Codecov by Sentry. |
gitttt-1234
requested review from
talmo,
eberrigan and
roomrys
and removed request for
talmo and
eberrigan
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/nn/inference.py(20 hunks)
🔇 Additional comments (7)
sleap/nn/inference.py (7)
734-738: Parameter input_scale added correctly to CentroidCropGroundTruth
The input_scale parameter is appropriately added to the __init__ method and initialized, allowing for flexibility in image resizing before passing to the model.
Also applies to: 741-741
2015-2015: Parameter resize_input_image added correctly to FindInstancePeaks
The addition of the resize_input_image parameter provides control over image resizing during preprocessing, enhancing flexibility.
Also applies to: 2028-2028
2126-2126: Pass resize_img parameter correctly in preprocess
The resize_input_image flag is correctly utilized to control resizing within the preprocess method, ensuring images are only resized when intended.
2411-2416: Set input_scale and precrop_resize consistently based on use_gt_centroid
The conditional assignment of input_scale and precrop_resize ensures that image scaling is handled correctly depending on whether ground truth centroids are used.
4052-4052: Use resize_input_image flag consistently in preprocessing
Passing resize_img=self.resize_input_image maintains control over image resizing, and ensures consistency across preprocessing steps.
4301-4301: Correctly set resize_input_image to False
Disabling image resizing at this stage is appropriate for the TopDownMultiClassFindPeaks use case.
4303-4304: Proper assignment of input_scale for ground truth centroids
Setting centroid_crop_layer.input_scale when using ground truth centroids ensures that the images are appropriately scaled during inference.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
sleap/nn/inference.py (2)
730-741: Clarify the default param docstringThe docstrings mention the new input_scale parameter but do not explicitly note the default. Adding the default value (e.g., 1.0) in the docstring will help users understand the expected behavior.
3874-3878: Parameter documentation improvementThe docstring for resize_input_image is helpful. Consider referencing any interplay with precrop_resize here for completeness, especially when both might be used in the same pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sleap/nn/inference.py(21 hunks)sleap/nn/training.py(2 hunks)
🔇 Additional comments (6)
sleap/nn/training.py (1)
1318-1322: Parameter overrides risk overshadowing user configuration
Here, the code explicitly sets input_scale=1.0 and resize_input_image=False, which may prevent any user-specified values from taking effect. Ensure this is intentional and consistent with the rest of the pipeline configuration.
sleap/nn/inference.py (5)
768-770: Avoid in-place scaling of centroids
Modifying example_gt["centroids"] in place can lead to side effects if these centroids are used elsewhere. This repeats a previous recommendation about copying before scaling.
1834-1839: Ensure consistent handling of precrop_resize
This logic partially duplicates prior feedback about precrop_resize default values and checks (particularly checking for None vs. 1.0). Consider revisiting existing suggestions to unify the default and conditions around precrop_resize across the codebase.
1980-1984: Docstring clarity approved
The added docstrings describing resize_input_image are clear and sufficiently detailed.
2173-2175: Coordinate adjustment seems correct
The additional scaling of peak_points with crop_offsets and 1/self.input_scale appears logically consistent. Ensure that upstream computations rely on this adjusted coordinate system.
3920-3920: No issues detected at this line
No concerns with the newly introduced or modified code on this specific line.
There was a problem hiding this comment.
It works as expected during my manual tests - it would be nice to have some automated tests as well (which might involve adding a fixture for a trained model with the centered-instance scaled and ensuring that the predictions line up relatively well with the known body part locations).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/training_config.json (2)
162-164: Consider increasing validation batches for better model evaluation.The current configuration uses only 1 validation batch (
val_batches_per_epoch: 1), which might not provide a representative evaluation of the model's performance.Consider increasing
val_batches_per_epochto at least 10% of training batches (10 batches in this case).
129-156: Consider enabling data augmentation for better model generalization.All augmentation options are disabled (
false). While this might be intentional for testing, enabling some basic augmentations could improve model robustness.Consider enabling basic augmentations like rotation and translation:
"rotate": false, + "rotate": true, "rotation_min_angle": -180.0, "rotation_max_angle": 180.0, "translate": false, + "translate": true,tests/nn/test_inference.py (1)
720-720: Fix boolean comparison styleReplace equality comparisons with
Truewith direct boolean expressions for better readability and maintainability.Apply this diff:
-assert predictor.is_grayscale == True +assert predictor.is_grayscaleAlso applies to: 745-745
🧰 Tools
🪛 Ruff (0.8.2)
720-720: Avoid equality comparisons to
True; useif predictor.is_grayscale:for truth checksReplace with
predictor.is_grayscale(E712)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/training_log.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
sleap/nn/inference.py(21 hunks)tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/initial_config.json(1 hunks)tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/training_config.json(1 hunks)tests/fixtures/models.py(1 hunks)tests/nn/test_inference.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/nn/test_inference.py
720-720: Avoid equality comparisons to True; use if predictor.is_grayscale: for truth checks
Replace with predictor.is_grayscale
(E712)
745-745: Avoid equality comparisons to True; use if predictor.is_grayscale: for truth checks
Replace with predictor.is_grayscale
(E712)
🔇 Additional comments (9)
tests/fixtures/models.py (1)
15-18: LGTM! The fixture follows the established pattern.
The new fixture min_centered_instance_with_scaling_model_path is well-implemented and maintains consistency with other fixtures in the file.
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/initial_config.json (1)
18-24: Verify the input scaling configuration.
The input_scaling parameter is set to 0.5 and resize_and_pad_to_target is enabled, but target_height and target_width are null. This combination might lead to undefined behavior.
Run the following script to check for similar configurations in other models:
✅ Verification successful
Having null target dimensions with resize_and_pad_to_target=true is the expected configuration
Based on the verification results, this is actually the standard configuration pattern in the codebase. We found two model configurations that use resize_and_pad_to_target=true, and both follow the exact same pattern where target_height and target_width are set to null. The input scaling values vary (0.5 and 1.0) but the null target dimensions are consistent, suggesting this is the intended design.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check other model configurations for input scaling patterns
# Look for files that have both input_scaling and resize_and_pad_to_target
fd 'initial_config.json' | xargs rg -l 'input_scaling.*resize_and_pad_to_target' | while read -r file; do
echo "=== $file ==="
jq '.data.preprocessing | {input_scaling, resize_and_pad_to_target, target_height, target_width}' "$file"
done
Length of output: 95
Script:
#!/bin/bash
# Let's try a different approach by searching for each field separately
echo "=== Files with input_scaling ==="
fd 'initial_config.json' | xargs jq -r 'select(.data.preprocessing.input_scaling != null) | .data.preprocessing.input_scaling'
echo -e "\n=== Files with resize_and_pad_to_target ==="
fd 'initial_config.json' | xargs jq -r 'select(.data.preprocessing.resize_and_pad_to_target == true) | .data.preprocessing.resize_and_pad_to_target'
echo -e "\n=== Files with target dimensions ==="
fd 'initial_config.json' | xargs jq -r 'select(.data.preprocessing.target_height != null or .data.preprocessing.target_width != null) | .data.preprocessing | {target_height, target_width}'
Length of output: 761
Script:
#!/bin/bash
# Let's get the full context of configurations that have resize_and_pad_to_target=true
echo "=== Full preprocessing configs with resize_and_pad_to_target=true ==="
fd 'initial_config.json' | while read -r file; do
echo "=== $file ==="
jq -r 'select(.data.preprocessing.resize_and_pad_to_target == true) | .data.preprocessing' "$file"
done
Length of output: 3162
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/training_config.json (1)
74-83: LGTM! Input scaling configuration is properly set.
The preprocessing configuration correctly specifies:
- Input scaling factor: 0.5
- Target dimensions: 384x384
- Padding stride: 1
tests/nn/test_inference.py (2)
708-730: LGTM! Test coverage for TopDownPredictor with scaling
The test function properly validates the behavior of TopDownPredictor with scaling by:
- Initializing the predictor with a centered instance model that has scaling
- Verifying the grayscale property
- Checking the number of instances and their points match expectations
- Validating point coordinates are within tolerance
🧰 Tools
🪛 Ruff (0.8.2)
720-720: Avoid equality comparisons to True; use if predictor.is_grayscale: for truth checks
Replace with predictor.is_grayscale
(E712)
732-755: LGTM! Test coverage for TopDownPredictor with both models and scaling
The test function thoroughly validates the behavior of TopDownPredictor when using both centroid and centered instance models with scaling by:
- Initializing the predictor with both models
- Verifying the grayscale property
- Checking the number of instances and their points match expectations
- Validating point coordinates are within tolerance
🧰 Tools
🪛 Ruff (0.8.2)
745-745: Avoid equality comparisons to True; use if predictor.is_grayscale: for truth checks
Replace with predictor.is_grayscale
(E712)
sleap/nn/inference.py (4)
730-731: LGTM! Input scaling support in CentroidCropGroundTruth
The implementation correctly adds input scaling support to CentroidCropGroundTruth by:
- Adding clear documentation for the input_scale parameter
- Adding input_scale parameter with default value of 1.0
- Implementing scaling functionality that resizes images and adjusts centroids
Also applies to: 734-741, 768-770
1652-1654: LGTM! Pre-crop resizing support in CentroidCrop
The implementation correctly adds pre-crop resizing support to CentroidCrop by:
- Adding clear documentation for the precrop_resize parameter
- Adding precrop_resize parameter with default value of 1.0
- Implementing resizing functionality that resizes full images and adjusts centroid points
Also applies to: 1695-1695, 1716-1716, 1835-1841
1982-1986: LGTM! Resize control in FindInstancePeaks
The implementation correctly adds resize control to FindInstancePeaks by:
- Adding clear documentation for the resize_input_image parameter
- Adding resize_input_image parameter with default value of True
- Modifying preprocess call to respect resize_input_image flag
- Adjusting peak points correctly based on input scale
Also applies to: 2017-2017, 2030-2030, 2128-2128, 2175-2177
3877-3881: LGTM! Resize control in TopDownMultiClassFindPeaks
The implementation correctly adds resize control to TopDownMultiClassFindPeaks by:
- Adding clear documentation for the resize_input_image parameter
- Adding resize_input_image parameter with default value of True
- Modifying preprocess call to respect resize_input_image flag
- Setting resize_input_image to False and configuring input_scale correctly when used with CentroidCropGroundTruth
Also applies to: 3923-3923, 3939-3939, 4057-4057, 4306-4306, 4308-4309
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/training_config.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/training_config.json (4)
93-102: Consider model capacity and precision trade-offs.The current configuration has some notable characteristics that might affect model performance:
- Small initial filters (16) with 1.5x growth rate
- Aggressive output stride of 2
- Offset refinement enabled
While these settings might be suitable for a minimal test configuration, they could impact model capacity and precision in production scenarios.
Consider adjusting these parameters if you need better precision or model capacity in production.
Also applies to: 110-120
130-155: Consider enabling data augmentations for better model robustness.All data augmentations (rotation, translation, scale, noise, etc.) are currently disabled. While this might be intentional for testing, enabling appropriate augmentations could help:
- Improve model robustness
- Prevent overfitting
- Handle variations in input data better
Consider enabling relevant augmentations based on your use case characteristics.
196-213: Consider enabling debugging features during development.Several debugging features are disabled:
- save_visualizations: false
- tensorboard.write_logs: false
- tensorboard.architecture_graph: false
Enabling these during development could help diagnose potential issues with the input scaling fix.
222-223: Add descriptive metadata.The name and description fields are empty. Consider adding:
- A descriptive name indicating this is a minimal instance configuration with scaling
- A description explaining the purpose and expected usage of this configuration
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/training_config.json(1 hunks)
🔇 Additional comments (1)
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/training_config.json (1)
78-82: Verify input scaling and resize configuration.
The configuration shows both input scaling (0.5) and resize_and_pad_to_target (true) being used together. This combination needs careful consideration:
- Input images will first be scaled by 0.5
- Then resized to 384x384
Please verify that this is the intended behavior for fixing the input scaling issue mentioned in #1993.
| "name": "", | ||
| "description": "", | ||
| "sleap_version": "1.4.1", | ||
| "filename": "models\\minimal_instance.UNet.centered_instance_with_scaling\\training_config.json" |
There was a problem hiding this comment.
Replace Windows path separator with forward slashes.
The filename uses Windows-style backslashes which could cause issues on other platforms.
- "filename": "models\\minimal_instance.UNet.centered_instance_with_scaling\\training_config.json"
+ "filename": "models/minimal_instance.UNet.centered_instance_with_scaling/training_config.json"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "filename": "models\\minimal_instance.UNet.centered_instance_with_scaling\\training_config.json" | |
| "filename": "models/minimal_instance.UNet.centered_instance_with_scaling/training_config.json" |
There was a problem hiding this comment.
We don't necessarily need this file
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/labels_gt.val.slp
Outdated
Show resolved
Hide resolved
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/labels_pr.train.slp
Outdated
Show resolved
Hide resolved
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/labels_pr.val.slp
Outdated
Show resolved
Hide resolved
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/metrics.train.npz
Outdated
Show resolved
Hide resolved
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/metrics.val.npz
Outdated
Show resolved
Hide resolved
tests/data/models/minimal_instance.UNet.centered_instance_with_scaling/training_log.csv
Outdated
Show resolved
Hide resolved
Description
This PR fixes the issue with input scaling in centered-instance model addressed here.
resizingto themake_viz_pipeline.CentroidCropGroundTruthwhile running inference on a centered-instance model, instead of resizing the crops inFindInstancePeaksclass.Types of changes
Does this address any currently open issues?
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
TopDownPredictorwith scaling.