-
Notifications
You must be signed in to change notification settings - Fork 15
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
Complete cell-SAM with two channel selection #530
base: main
Are you sure you want to change the base?
Conversation
@mimithecoconut I went ahead and pushed up reversions of the windows line endings that were extraneously added due to a bad editor config at some point. It looks like it was only affecting old files so hopefully the updated configuration makes this a non-problem moving forward; it might be worth double-checking your editor configuration though! |
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.
Thanks for this @mimithecoconut !
I went ahead and did a first pass - I think the first thing we need to do to get this closer to mergeable is to do some more cleanup to ensure that only the changes that are relevant to cellsam are captured in this PR.
From a quick readthrough, it seems like a lot of the changes (to cypress, for instance) are related to different features and are not related to cellsam. The most common cause of issues like this is that the branch was not created from a "clean" starting point; i.e. the cellSAMfinal
branch was accidentally created from a starting point other than main
, or you had accidentally already committed changes to main
locally before creating the branch. There are a couple different things we can do about this... one would be to create a new branch and cherry-pick
only the commits that contain changes related to cellsam onto that branch. I already went ahead and pushed up a bunch of cosmetic changes to this branch, so I'd actually vote to just go in and manually undo any unintended changes (see the inline comments) and push them up here.
There's still a few other things to work out before we can merge, but the very first step is to winnow this down so that we're 100% sure we're only looking at the changes that are necessary for cellsam integration.
Also a reminder: I've pushed fixes to this branch so make sure to pull
before pushing up your next set of changes!
frontend/src/Project/DisplayControls/LabeledControls/CellsOpacitySlider.js
Outdated
Show resolved
Hide resolved
frontend/src/Project/EditControls/SegmentSamControls/ActionButtons.js
Outdated
Show resolved
Hide resolved
NOTE: requires more work to extract this info from environment.
# Cellsam server | ||
CELLSAM_IP=127.0.0.1 | ||
CELLSAM_PORT=8765 | ||
CELLSAM_SERVER_VERSION= |
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.
@rdilip for now, let's record the commit hash of the cellsam compute server that works with this version of deepcell-label. In principle we can use tags as well, which may make more sense once the formatting of messages etc. is completely decided.
The most elegant way to do this would be to include the cellsam server as a submodule in this repo; however, mixing public and private repos in this way is likely to cause problems (I don't even know if/how it's supported). Nevertheless, something to keep in mind if the cellsam code is released in the future.
Can select two main channels to connect to cell-SAM and generate masks.