Skip to content
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 processing tests #939

Conversation

SylviaWhittle
Copy link
Collaborator

This PR (hopefully) fixes the tests in test_processing.py for the #932

@SylviaWhittle
Copy link
Collaborator Author

Ah heck, so this PR is just to address the processing tests, not the others... Any ideas on how to do a PR that fixes only some of the tests? @ns-rse

@SylviaWhittle
Copy link
Collaborator Author

The processing tests pass on my machine, you might want to verify though
image

@ns-rse
Copy link
Collaborator

ns-rse commented Oct 10, 2024

Any ideas on how to do a PR that fixes only some of the tests? @ns-rse

The Continuous Integration tests will continue to fail until all tests pass, no way around that I can think of without switching to using pytest --testmon in CI if that is even possible as it would be problematic if say a newer package were installed in CI than on a local machine where the .testmondata.

Its useful to run the whole test suite in CI though and ensuring it runs on all pull requests was something I deliberately introduced the other day with #921 as had this been the case from the start (rather than just running on PRs that target main) we would have known about and addressed the failing tests earlier.

Copy link
Collaborator

@MaxGamill-Sheffield MaxGamill-Sheffield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and TS runs!

@SylviaWhittle SylviaWhittle merged commit f890e6d into maxgamill-sheffield/800-better-tracing Oct 10, 2024
0 of 10 checks passed
@SylviaWhittle SylviaWhittle deleted the SylviaWhittle/800-tests-processing branch October 10, 2024 12:17
@@ -295,6 +300,41 @@ def remove_small_objects(self, image: np.array, **kwargs) -> npt.NDArray:
return small_objects_removed > 0.0
return image

def remove_objects_too_small_to_process(
self, image: npt.NDArray, minimum_size_px: int, minimum_bbox_size_px: int
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect this method to be used independently of the processing?

If not then I don't see any advantage have minimum_size_px or minimum_bbox_size_px as arguments to the method as they are set as attributes to the class when it is initialised and available as self.minimum_grain_size_px and self.minimum_bbox_size_px.

The comment above on hardcoding suggests its not something that will ever be changed.

bbox_width = region.bbox[2] - region.bbox[0]
bbox_height = region.bbox[3] - region.bbox[1]
# If the minimum dimension of the bounding box is less than the minimum dimension, remove the region
if min(bbox_width, bbox_height) < minimum_bbox_size_px:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However unlikely to get an almost straight line is it possible that there would ever be a bounding box with one dimension < 5 and the other say 400 pixels?

Would such an image not be processed by DNA tracing?

Could perhaps have such a dummy grain as a test scenario to check (but I appreciate this is additional work).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants