fix(plotting): remove dilation if images are large #914
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The section which automatically dilated masks had been commented out as part of the refactoring for `better-tracing`. This meant that the related test (`tests/test_plottingfuncs.py::test_mask_dilation()`) failed. After discussion it was decided to remove this functionality completely. We have left the `topostats.plottingfuncs.dilate_binary_image()` which may be used elsewhere. It isn't currently but there _is_ an argument/option in `default_config.yaml` under `mask_smoothing_params` which indicates the dilation is undertaken during smoothing so perhaps we can simplify the code by calling the `dilate_binary_image()` function we have in place with tests and reduce the amount of code within the smoothing module. Further the step which turns a mask with numbered layers to binary (`0`/`1`) has been removed because plots now require there to be different layers of masking to differentiate individual molecules. Another issue is that the tests were developed with a value of `zrange: [null, null]` but in this branch the `default_config.yaml` has been modified to `zrange: [-2, 6]` which results in six tests failing. The `default_config.yaml` has been reverted to `zrange: [null, null]`. This does raise a couple of issues. 1. What should the defaults be in the `main` branch and in releases? When developing features and testing new code it may be desirable to use custom configurations but what should we be using as defaults for releases? What set of values works best for _most_ use cases? 2. Use `--config <custom_config>.yaml` Directly related to this is that we should not be changing values in `default_config.yaml` when undertaking development. Rather we should use the `--config` flag with a custom configuration file. When new options need adding that isn't a problem to add them to `default_config.yaml` but when modifying values it is important to ensure all tests pass. It can take some time to track down and work out the root cause of these test failures!
ns-rse
requested review from
SylviaWhittle,
MaxGamill-Sheffield and
llwiggins
MaxGamill-Sheffield
approved these changes
Sep 27, 2024
ns-rse
merged commit 1fa9988
into
maxgamill-sheffield/800-better-tracing
2 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The section which automatically dilated masks had been commented out as part of the refactoring for
better-tracing. This meant that the related test (tests/test_plottingfuncs.py::test_mask_dilation()) failed.After discussion it was decided to remove this functionality completely. We have left the
topostats.plottingfuncs.dilate_binary_image()which may be used elsewhere. It isn't currently but there is an argument/option indefault_config.yamlundermask_smoothing_paramswhich indicates the dilation is undertaken during smoothing so perhaps we can simplify the code by calling thedilate_binary_image()function we have in place with tests and reduce the amount of code within the smoothing module.Further the step which turns a mask with numbered layers to binary (
0/1) has been removed because plots now require there to be different layers of masking to differentiate individual molecules.Another issue is that the tests were developed with a value of
zrange: [null, null]but in this branch thedefault_config.yamlhas been modified tozrange: [-2, 6]which results in six tests failing. Thedefault_config.yamlhas been reverted tozrange: [null, null].This does raise a couple of issues.
mainbranch and in releases?When developing features and testing new code it may be desirable to use custom configurations but what should we be using as defaults for releases? What set of values works best for most use cases?
--config <custom_config>.yamlDirectly related to this is that we should not be changing values in
default_config.yamlwhen undertaking development. Rather we should use the--configflag with a custom configuration file. When new options need adding that isn't a problem to add them todefault_config.yamlbut when modifying values it is important to ensure all tests pass.It can take some time to track down and work out the root cause of these test failures!
This partially address the issues highlighted in #850.