comic / evalutils

evalutils helps users create extensions for grand-challenge.org

https://grand-challenge.org

MIT License

23 stars 9 forks source link

Added use_memory_efficient_edt switch to stats methods #311

Closed silvandeleemput closed 3 years ago

silvandeleemput commented 3 years ago

Closes #303

This PR

Adds a use_memory_efficient_edt switch to all methods computing edt transforms in stats.py.
The default for the flag is set to the (non-memory-efficient) scipy.ndimage.morphology.distance_transform_edt version.
When enabled it uses the distance_transform_edt_float32 version in stats.py.
Docstring has been added for the new parameter as well

@jmsmkn The current default was the memory-efficient version, but now it will use the (non-memory-efficient) scipy.ndimage.morphology.distance_transform_edt version. I figured that it would be more desirable to move away from custom methods. Is it ok to break it in this way, or would we rather put the default to the memory-efficient version we used before?

jmsmkn commented 3 years ago

Default should be to use the memory efficient version.

silvandeleemput commented 3 years ago

This PR should be merged or rebased to master after https://github.com/comic/evalutils/pull/313 lands.

jmsmkn commented 3 years ago

Looking at the constructions, wouldn't it be nicer to pass edt_method around rather than have a boolean option and then a few switches everywhere, then default that to the one included in this library.

silvandeleemput commented 3 years ago

That would be both cleaner and more flexible, but as an interface for the user, I think you only would ever want to change between those two methods, hence a boolean should be simpler and sufficient.

However, if you think that directly supplying the edt_method is better, I would be happy to change it.

codecov[bot] commented 3 years ago

Codecov Report

Merging #311 (a798c07) into master (992241b) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #311   +/-   ##
=======================================
  Coverage   85.24%   85.24%           
=======================================
  Files          13       13           
  Lines         881      881           
  Branches      127      127           
=======================================
  Hits          751      751           
  Misses         87       87           
  Partials       43       43

Impacted Files	Coverage Δ
evalutils/stats.py	`86.30% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 992241b...a798c07. Read the comment docs.

jmsmkn commented 3 years ago

No strong opinions about it, in general I think it's a better pattern and was just thinking to get it right now as public interfaces should never change.