Skip to content
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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

mimithecoconut
Copy link

Can select two main channels to connect to cell-SAM and generate masks.

@rossbar
Copy link
Contributor

rossbar commented Dec 8, 2023

@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!

Copy link
Contributor

@rossbar rossbar left a 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!

backend/.gitignore Outdated Show resolved Hide resolved
backend/deepcell_label/label.py Outdated Show resolved Hide resolved
backend/deepcell_label/loaders.py Outdated Show resolved Hide resolved
backend/deepcell_label/loaders.py Outdated Show resolved Hide resolved
backend/deepcell_label/loaders.py Outdated Show resolved Hide resolved
cypress/support/commands.js Outdated Show resolved Hide resolved
cypress/support/e2e.js Outdated Show resolved Hide resolved
frontend/src/Project/cells.ts Outdated Show resolved Hide resolved
# Cellsam server
CELLSAM_IP=127.0.0.1
CELLSAM_PORT=8765
CELLSAM_SERVER_VERSION=
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants