Colocalization cellmap tweak #161
Conversation
…example it allows to select a unique representative in each connect component of maxima.
…ble detection of cells.
…_detection, to avoid buggy use of watershedding.
…e in comments for python >= 3.11
…ll to voxelize could yield nonseparated peaks and cause bugs
…dding in shape detection
…ink with source in IO.initialize
…tem__ by "key in .." check.
There was a problem hiding this comment.
Changing the API defaults is dangerous as this may create unexpected behaviour. Whenever possible, try to reproduce the expected behaviour by default and the new behaviour when parameters are passed (unless this is a bugfix)
There was a problem hiding this comment.
Same try to preserve exsiting behaviour by default unless you are fixing a but
There was a problem hiding this comment.
watershed_line=True should be function of arguments
There was a problem hiding this comment.
I added watershed_line as an argument. The ancient behavior would be with watershed_mask=False.
Yet, for consistency of the new version, to have the sizes of detected cells independent of the other arguments choices, the default watershed_mask = True is the only choice.
Moreover the difference between False and True for watershed_mask does not entail a drastic change in the observed sizes.
I would favor consistency of the new version against maintaining the exact same API. Feel free to make your choice.
There was a problem hiding this comment.
Typo introduced in docstring
Are you actually initialising the asset to None ?
The idea is that one might search for an asset with a prefix (not an exact match)
There was a problem hiding this comment.
new lines 310-312 : this is actually a fix for the case there would be no entry for asset_type is in the AssetCollection self.asset_collections[channel] of former line 310. In that case the getitem method would raise an exception.
typo fixed
There was a problem hiding this comment.
you are not checking for presave_parser before using it.
If it is automatically required whenever an other argument is selected, verify that it is the case and raise a clearer exception otherwise
There was a problem hiding this comment.
got it. I made the change.
There was a problem hiding this comment.
dtype = par.get('save_dtype', 'float')
There was a problem hiding this comment.
Better to preserve behaviour whenever possible through additive changes
It might make sense to signal a change of behaviour otherwise
ln430 and 462 are not indented as expected
ln466 is blank
There was a problem hiding this comment.
checked perf of cell-detection with ancient and new implementation
with cfos channel of your test dataset, on my workstation.
6'16'' with ancient
6''25'' with new
Since the new implementation prevents counting twice the same cell in the case of adjacent maxima, I think this new version should be merged.
follow expanduser() demand.
added missing presave_parser check
fixed default to initial value
fix indent
FORMAT: add 3 spaces
FORMAT: add one space
FORMAT: remove blank line.
Fix typo
There was a problem hiding this comment.
dtype = par.get('save_dtype', 'float')
There was a problem hiding this comment.
watershed_line=True should be function of arguments
simplify par.get call with correct default value usage
removed hardcoded path
Remove debug print and add watershed_line to detect_shape signature.
use watershed_line input in detect_shape
Cellmap tweak to have the correct output to instantiate colocalization.Channel.