-
Notifications
You must be signed in to change notification settings - Fork 49
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
Colocalization cellmap tweak #161
base: v3.0.x
Are you sure you want to change the base?
Changes from 31 commits
33365ef
7f681dd
a0abf03
d56dd7c
d79c41f
23ac737
88fd9ad
9d2e942
5f26926
20c7922
308136b
1a54e3f
96021d3
95e42d2
ec379e6
2d17b9f
2081d41
dca31b8
2974efb
e1810f7
cd546f6
3c8fa6a
2fcaad7
7a849f6
9f2fc44
361a37a
aa9c21c
ade8785
8cc6e44
3e09651
710ff71
2f7e732
a71709f
08be8d6
670e6ff
74f0811
bcfb9df
29fbdca
e062f8f
f9fd289
035b947
74a1647
5e3d18d
347ed5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo introduced in docstring There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better to preserve behaviour whenever possible through additive changes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check performance There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. checked perf of cell-detection with ancient and new implementation |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you are not checking for presave_parser before using it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it. I made the change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dtype = par.get('save_dtype', 'float') There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same try to preserve exsiting behaviour by default unless you are fixing a but
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
watershed_line=True should be function of arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.