-
Notifications
You must be signed in to change notification settings - Fork 102
🐛 Fix sccnn Model
#970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 Fix sccnn Model
#970
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes critical issues with the SCCNN model where preprocessing was being applied twice - once in the dataloader and once in the model's forward function - causing the model to output zeros due to values being divided by 255 twice.
Key changes:
- Removed duplicate preprocessing call from the model's forward function
- Changed preproc method signature from torch.Tensor to np.ndarray to match actual usage
- Updated model configurations to use correct resolution (0.25 mpp instead of 0.5 mpp)
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tiatoolbox/models/architecture/sccnn.py | Fixed preproc typing to use np.ndarray and removed duplicate preproc call from forward method |
| tiatoolbox/data/pretrained_model.yaml | Updated resolution from 0.5 to 0.25 mpp for both sccnn-crchisto and sccnn-conic models |
| tests/models/test_arch_sccnn.py | Added explicit preproc call before infer_batch and improved assertions using numpy testing functions |
Comments suppressed due to low confidence (1)
tiatoolbox/models/architecture/sccnn.py:254
- The docstring for the
preprocmethod needs to be updated to reflect the signature change. The Args and Returns sections still referencetorch.Tensor, but the method signature now usesnp.ndarrayfor both input and output.
Suggested fix:
"""Transforming network input to desired format.
This method is model and dataset specific, meaning that it can be replaced by
user's desired transform function before training/inference.
Args:
image (np.ndarray): Input images, the array is of the shape HWC.
Returns:
output (np.ndarray): The transformed input.
""" """Transforming network input to desired format.
This method is model and dataset specific, meaning that it can be replaced by
user's desired transform function before training/inference.
Args:
image (torch.Tensor): Input images, the tensor is of the shape NCHW.
Returns:
output (torch.Tensor): The transformed input.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #970 +/- ##
===========================================
- Coverage 99.27% 99.27% -0.01%
===========================================
Files 71 71
Lines 9162 9161 -1
Branches 1195 1195
===========================================
- Hits 9096 9095 -1
Misses 40 40
Partials 26 26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tiatoolbox/models/architecture/sccnn.py:252
- The docstring for
preprocstill referencestorch.Tensortypes in the Args and Returns sections, but the method signature has been updated to usenp.ndarray. The documentation should be updated to match:image (np.ndarray): Input images, the array is of the shape HWC or NCHW.andnp.ndarray: The transformed input.
Args:
image (torch.Tensor): Input images, the tensor is of the shape NCHW.
Returns:
output (torch.Tensor): The transformed input.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shaneahmed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Jiaqi-Lv
This pull request fixes issues with the SCCNN model. Solving issue #969 .
Firstly, the unit test is fixed by adding
model.preproc(patch)step before callinginfer_batch(), pre-processing function is supposed to be applied on the input patch before the model's forward pass in our engines.In the unit test,
assertis changed using numpy's assert function to compare two arrays. Previously what happened was the output could be an empty numpy array of shape(0,2), the assert would still returnTrue, which was wrong and unsafe. Example:I removed
preprocfrom SCCNN'sforward()function. It should not be used here.Finally, I updated the input resolution for sccnn models, as from the unit test the resolution seems to be 0.25 mpp.
Regarding issue #969 : Previously the test passes, but as
preprocwas applied in the model'sforwardfunction. When I tried to run the model inside theSementicSegmentorengine,preprocwas essentially ran twice, once from thedataloader, and once again in thesccnn.forward()function, which meant the input was divided by 255 twice, therefore the model only outputs zeros as the values were too small.