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.
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
Develop #218
base: develop_2
Are you sure you want to change the base?
Develop #218
Changes from 72 commits
938398a
7a53648
8885304
bb46eb3
bc527b8
df532c7
3c6cf13
886b367
a51ba29
5110842
85a7f13
ea1e5e4
5cc8224
8078ec4
db38859
179f97f
96de30c
6f319ec
211f7f9
f5acee9
f94457c
ddedab1
cf24b71
cac68a8
275032c
cd0b42f
db53400
8fc8a82
cc9e0f3
fd393ec
5787d9a
37db81e
dd45ac7
acd5ff8
4188719
6da23e7
9773717
ddeb92a
bfefc72
16b54c3
afb33ec
3143f06
791cf27
c649834
eeeb40a
08081e7
f29924c
23fdf83
4b7ee5f
2bb109e
d29803d
cabfcc7
697cd98
67eaa34
736ed0b
b2acf9c
cbc3937
cb0cbd2
436d671
bb24837
5263843
1c09ce6
0d6a406
a4f99b5
00ca127
8ee813f
c6590aa
5aee184
464ca32
34da6ef
e81c996
67bdbd9
3fc4fe1
6869c7f
66b5c11
cb7f94e
7a21bb0
17b47a8
5cf0a10
e5910e7
9254ae2
aa78c3c
5550339
b440b80
76aad64
fffefdf
bd156ae
c92be38
3a184fd
b8947fd
27c99d2
4cb3da1
a5fba4c
a035b7b
b558aec
22ad48f
8cf35de
983bb19
94e315a
ae5ecde
c7d9b0c
a6bf141
aac7b07
72d00d3
dec0151
fb373b4
4683975
9306345
e843deb
3ef61bd
46602d6
bf25fbe
93012e9
0d05847
c21376a
6e66670
f82bc18
d4b6a24
588307f
dc4a0c9
88b16b0
14fc238
07ec73e
91dd95f
d232516
41edff4
49545cf
7b73f29
adf303b
42a69ce
676664f
c471ce9
e459155
2ef58e9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Also, preprocessing images "generates individual txt files"? This doesn't sound very intuitive to me, without knowing anything about the functionality or the new file format.
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.
There is no new format, just lack of txt backups, does
steinbock export txt
make more sense?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.
Are you sure that link works? I may be wrong, but the double
##
looks suspiciousThere 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.
corrected
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.
Nice! Where can I find these parameter values?
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.
steinbock segment cellpose run --help
would display the cellpose defaults, are you suggesting that an explicit list should be kept somewhere?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.
Perhaps mention explicitly how to list the default parameter values here?
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.
Bit repetitive:
Also, the "see XYZ" should probably be links.
Further, I find this a bit contradictory:
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.
addressed the repetition and removed the second block. This can be considered as redundant. However this describes what cellpose does in case you specify models both via -pretrained-model and -model-type, in that case the -model-type takes precedence. This is my understanding, but I think I leave commentary on this to cellpose itself.
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.
This does not match the code interface. I think it should be
steinbock segment cellpose train ...
?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.
leftover from before the change, updated!
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.
In general, I think the parameter spelling in the documentation need to be double-checked (e.g., replace
_
by-
)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.
corrected
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.
Shouldn't it be
--pretrained-model
?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.
corrected
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.
Really?
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.
The syntax of this link looks wrong to me?
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.
corrected.
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.
Without checking in detail, shouldn't there be
2 ** 3 = 8
different flavors? Also, perhaps its easier to only explain the 3 tags (cellpose, gpu, xpra), instead of showing all the combinations (especially, since e.g. using cellpose-gpu and cellpose-gpu-xpra currently doesn't make much sense)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.
revised
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.
Why is this needed? Isn't opencv-python-headless enough?
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.
Revising the comments, I realize this follows from #218 (comment). Still, it remains unclear to me where the opencv-python dependency comes from.
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.
@Milad4849 Any update on why this dependency is required?
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.
Last time checked required on my system, thorough testing on multiple machines will be done once the develop branch is ready.
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.
This looks wrong
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.
changed to 'python'
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.
This looks wrong
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.
changes to 'python'
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.
Why not allow arguments to the cellpose command?
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.
My idea was to leave everything to be specified from the gui.
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.
Ok! I don't see a harm in supporting the cellpose command-line parameters, too (especially since you don't need to specify them explicitly when using
click.UNPROCESSED
), but your choice.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.
Changed to support unprocessed args for cellpose
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 also made xti file handling with the last commit as well, in brief if xti -> individual txt files are generated for the ROIs (the xti machine does not output txt backups). Added this last minute as it is high priority and currently needed.
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.
Ok, I won't review this conceptually, as I'm not familiar with the XTi. But perhaps this should be added to the online documentation?
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.
Added
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.
Without knowing too much about this: Why generate txt files instead of images (especially, since the command is called
preprocess images
)? Also, doesn't generating txt files AFTER extracting images destroy the whole txt-for-broken-mcd recovery mechanism implemented above?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.
In brief, downstream analysis relies on the txt files which are not provided by the new xti IMC machines.Therefore the txt files are not meant as back ups. It is a feature requested exactly in this format:
steinbock preprocess --xti
but perhaps best implemented as a part ofexport
? At any rate, the txt files have to be generated directly from the mcd to avoid any unwanted changes.