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

Comseg cli #76

Merged
merged 31 commits into from
Jul 4, 2024
Merged

Comseg cli #76

merged 31 commits into from
Jul 4, 2024

Conversation

tdefa
Copy link
Contributor

@tdefa tdefa commented Jun 5, 2024

implementation of ComSeg cli

  • An example to run comseg in CLI is in sopa/docs/tutorials/cli_other_segmentation.md
  • documentation at "sopa/docs/cli.md" was update fro comseg

A function transcript_segmentation() was created to handle both Baysor and ComSeg

Copy link
Collaborator

@quentinblampey quentinblampey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looking good! I added a few comments

docs/tutorials/cli_other_segmentation.md Show resolved Hide resolved
sopa/cli/segmentation.py Outdated Show resolved Hide resolved
sopa/cli/segmentation.py Outdated Show resolved Hide resolved
sopa/cli/segmentation.py Outdated Show resolved Hide resolved
sopa/cli/patchify.py Outdated Show resolved Hide resolved
sopa/cli/patchify.py Outdated Show resolved Hide resolved
@quentinblampey
Copy link
Collaborator

quentinblampey commented Jun 6, 2024

Here is a todo list for the next steps:

  • Update the Snakefile and utils.py
  • Have a complete ComSeg tutorial for API + CLI + snakemake
  • Have a ComSeg config for the toy dataset (used in the above snakemake tutorial)
  • Have a ComSeg config for a few major technologies (e.g., MERSCOPE and Xenium)

@quentinblampey
Copy link
Collaborator

Thanks for the changes! Regarding the CI, you can install pre-commit locally to fix the issues:

pre-commit install

pre-commit run --all-files

@tdefa
Copy link
Contributor Author

tdefa commented Jun 11, 2024

Thanks for the changes! Regarding the CI, you can install pre-commit locally to fix the issues:

pre-commit install

pre-commit run --all-files

Ok thanks I just saw it

@tdefa
Copy link
Contributor Author

tdefa commented Jun 11, 2024

I added comseg to the snakemake pipeline + few config file examples.

Now I will complete ComSeg tutorial for API + CLI + snakemake all in the file : "other_segmentations.ipynb"
do you agree ?

@quentinblampey
Copy link
Collaborator

Thanks for your updates, it looks very good!

I fixed and cleaned a few things

Also, I had some issues running comseg, now I get an error 'PCA' object has no attribute 'explained_variance_' when running the patch step of comseg, do you know why?
Two other things:

  • Installing comseg is not enough to have all the dependencies, I also needed to install alphashape / igraph / leidenalg. Do you think these could be added as a dependency of ComSeg for convenience?
  • I moved the tutorial in docs/tutorial/comseg.ipynb, can you check and fill the "Snakemake" and "CLI" sections? It can be pretty short, since it is very similar to the existing tutorial. For instance, for the Snakemake pipeline, you can provide the command line to run the toy dataset, and for the CLI you can do something similar to the CLI tutorial with Baysor.
  • Why do you need to do importlib.reload(method)?

After this, I will merge the PR!

@tdefa
Copy link
Contributor Author

tdefa commented Jun 17, 2024

Ok thanks for reviewing!

  • PCA' object has no attribute 'explained_variance_' I will look at it and also complete dependency of ComSeg

  • for importlib.reload(method) I will remove it I think it is a mistake

I will do additional tests on other data this week, before the next push.

@tdefa
Copy link
Contributor Author

tdefa commented Jul 3, 2024

Hello,

Sorry it took so long, I think that it is now ready for your review.

I added min_transcripts_per_patch and min_cells_per_patch options to the CLI for ComSeg. Additionally, I included support for other segmentation than Cellpose for prior initialization, which is useful for protocols that already segment nuclei, such as Xenium. I also updated the Snakefile with checkpoint patchify_comseg. An example config for Snakemake + Xenium + ComSeg has been added.

I modified the valid index patches of ComSeg so it run only on crops with nuclei.

I updated the ComSeg dependency in the new pip version. However, I did not encounter the `PCA object has no attribute 'explained_variance_' issue, so I do not know why it occurred for you.

Copy link
Collaborator

@quentinblampey quentinblampey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me, thanks @tdefa!

@quentinblampey quentinblampey merged commit 5e2de67 into gustaveroussy:dev Jul 4, 2024
1 of 4 checks passed
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.

2 participants