fix: Fix preparing data for mahotas.haralick to avoid overflow problem#1150
fix: Fix preparing data for mahotas.haralick to avoid overflow problem#1150
mahotas.haralick to avoid overflow problem#1150Conversation
|
🧙 Sourcery has finished reviewing your pull request! Tips
|
WalkthroughThe changes primarily focus on modifying the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.
Czaki
changed the title
Czaki
changed the title
mahotas
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- package/PartSegCore/analysis/measurement_calculation.py (1 hunks)
- package/tests/conftest.py (1 hunks)
Additional comments not posted (2)
package/tests/conftest.py (1)
272-274: Review of test item reordering logicThe logic to separate and reorder test items based on the presence of "test_analysis_batch" in the file path appears to be correct. However, it's important to ensure that this reordering does not inadvertently affect dependencies between tests or the overall test suite's execution order.
Verification successful
Reordering of tests appears safe
The logic to separate and reorder test items based on the presence of "test_analysis_batch" in the file path appears to be correct. No direct dependencies on test order were found, and the use of fixtures does not indicate any explicit ordering constraints. Therefore, the reordering should not affect the overall test suite's execution.
- No instances of
pytest.mark.dependency,pytest.mark.order,pytest.mark.tryfirst, orpytest.mark.trylastwere found.- Several instances of
pytest.mark.usefixtureswere found, but they do not impose explicit order dependencies.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any test dependencies that might be affected by the reordering. # Test: Search for test dependencies. Expect: No direct dependencies on test order. rg --type python -A 5 $'pytest.mark.dependency'Length of output: 77
Script:
#!/bin/bash # Description: Check for any test dependencies that might be affected by the reordering. # Test: Search for test dependencies. Expect: No direct dependencies on test order. rg --glob '*.py' -A 5 $'pytest.mark.dependency'Length of output: 46
Script:
#!/bin/bash # Description: Check for other potential test dependencies or ordering constraints. # Test: Search for other potential dependencies or ordering constraints. Expect: No direct dependencies on test order. rg --glob '*.py' -A 5 $'pytest.mark.order' rg --glob '*.py' -A 5 $'pytest.mark.usefixtures' rg --glob '*.py' -A 5 $'pytest.mark.tryfirst' rg --glob '*.py' -A 5 $'pytest.mark.trylast'Length of output: 14159
package/PartSegCore/analysis/measurement_calculation.py (1)
1538-1538: Review of rescaling function modificationThe change from a divisor of 255 to 254 in the
_rescale_imagefunction is implemented correctly. However, it's crucial to verify that this change does not affect other functionalities that depend on the output of this function.
Czaki
changed the title
mahotasmahotas.haralick to avoid overflow problem
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1150 +/- ##
===========================================
- Coverage 93.04% 93.02% -0.02%
===========================================
Files 207 207
Lines 32700 32705 +5
===========================================
Hits 30425 30425
- Misses 2275 2280 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- package/PartSegCore/analysis/measurement_calculation.py (1 hunks)
Additional context used
GitHub Check: codecov/patch
package/PartSegCore/analysis/measurement_calculation.py
[warning] 1537-1537: package/PartSegCore/analysis/measurement_calculation.py#L1537
Added line #L1537 was not covered by tests
Additional comments not posted (1)
package/PartSegCore/analysis/measurement_calculation.py (1)
1536-1540: Adjustment in_rescale_imagefunction to prevent overflowThe change from 255 to 254 as a divisor in the rescaling formula is intended to prevent overflow issues. This is a sensible change given the context of the problem described in the PR. However, ensure that this change is reflected in all relevant parts of the application to maintain consistency.
Additionally, consider the impact this change might have on the visual representation of images. It would be beneficial to verify this through visual inspections or additional tests.
Tools
GitHub Check: codecov/patch
[warning] 1537-1537: package/PartSegCore/analysis/measurement_calculation.py#L1537
Added line #L1537 was not covered by tests
| if data.dtype == np.uint8: | ||
| return data | ||
| if isinstance(data.dtype, np.integer) and data.min() >= 0 and data.max() < 255: | ||
| return data.astype(np.uint8) |
There was a problem hiding this comment.
Missing test coverage for new line in _rescale_image
The static analysis tool flagged that the newly added line #1537 is not covered by tests. It's crucial to add unit tests for this change to ensure that the new behavior works as expected and does not introduce any regressions.
+ # Add unit tests to cover the new behavior in `_rescale_image`Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: codecov/patch
[warning] 1537-1537: package/PartSegCore/analysis/measurement_calculation.py#L1537
Added line #L1537 was not covered by tests
|
@coderabbitai review |
Actions performedReview triggered.
|
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)
- package/PartSegCore/analysis/measurement_calculation.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- package/PartSegCore/analysis/measurement_calculation.py
for more information, see https://pre-commit.ci
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)
- package/tests/conftest.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- package/tests/conftest.py
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)
- package/tests/conftest.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- package/tests/conftest.py
|
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)
- package/tests/conftest.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- package/tests/conftest.py
Czaki
changed the title
mahotas.haralick to avoid overflow problemmahotas.haralick to avoid overflow problem
1 participant
Workaround for luispedro/mahotas#150
Summary by CodeRabbit
Bug Fixes
Tests