Sylvia whittle/remove scars #390
Conversation
Codecov ReportBase: 76.88% // Head: 80.07% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #390 +/- ##
==========================================
+ Coverage 76.88% 80.07% +3.19%
==========================================
Files 32 36 +4
Lines 3638 3915 +277
==========================================
+ Hits 2797 3135 +338
+ Misses 841 780 -61
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Awesome work! Results look great!
The one thing most of my comments are about is whether this should be a separate class in filters.py, just because this is going to be used in the filters workflow and it seems a unusual to have a separate config block for this where it could be incorporated into the filters block and maybe a scar_removal subsection.
tests/_regtest_outputs/test_run_topostats.test_process_scan_lower.out
Outdated
Show resolved
Hide resolved
|
I have removed the I have also added a bit more documentation, though if anything isn't clear, please do say. With regards to using the If I haven't adequately addressed a previous concern, please do re-raise it. |
There was a problem hiding this comment.
Great work @SylviaWhittle saves the overhead of instantiating a class where most methods were @staticmethod and keeps things simple.
Most of my comments pertain to where configuration is being defined and the logic to control the running of scar removal.
The configuration doesn't have to be nested, but it makes things a bit simpler in my view as we wouldn't have to subset those out from the configuration dictionary and pass it around, they're a key/value within config_filter. This removes a few lines of code from run_topostats.py (keep code bloat down is good in my view).
SylviaWhittle
dismissed
MaxGamill-Sheffield’s stale review
Requested changes no longer relevant due to refactor away from using the Scars class.
Closes #202
Closes #154
This pull request adds the long awaited scar removal feature to TopoStats. Configuration for the scar removal has been added in the config file, and good/useful defaults have been set for the parameters.
The scar removal is done by the new
Scarsclass. Much like with theFiltersclass, its instantiation takes an image and the required parameters, then one can call theremove_scars()class method on the instance.remove_scars()is the only method intended to be used externally to the class, though it is written in a way that you could call other class methods if you really wanted to.There is a config option to turn off the scar removal if wanted.
This scar removal has been deemed to be of good enough quality, and it has been tested on a reasonable sample of scarred images, however with more usage, bugs may still be found.
An example of what the scar removal in action:
With scars:
Scars removed:
