Skip to content

Seurat method for label projection - #533

Merged
scottgigante-immunai merged 29 commits into
openproblems-bio:mainfrom
mxposed:label-projection-seurat
Oct 6, 2022
Merged

Seurat method for label projection#533
scottgigante-immunai merged 29 commits into
openproblems-bio:mainfrom
mxposed:label-projection-seurat

Conversation

@mxposed

@mxposed mxposed commented Aug 4, 2022

Copy link
Copy Markdown
Collaborator

Submission type

  • This submission adds a new dataset
  • This submission adds a new method
  • This submission adds a new metric
  • This submission adds a new task
  • This submission adds a new Docker image
  • This submission fixes a bug (link to related issue: )
  • This submission adds a new feature not listed above

Testing

  • This submission was written on a forked copy of SingleCellOpenProblems
  • GitHub Actions "Run Benchmark" tests are passing on this base branch of this pull
    request (include link to passed test: https://tower.nf/orgs/openproblems-bio/workspaces/openproblems-bio/watch/3EOkXLUlZ4fHJ8)
  • If this pull request is not ready for review (including passing the "Run
    Benchmark" tests), I will open this PR as a draft (click on the down arrow next to the
    "Create Pull Request" button)

Submission guidelines

  • This submission follows the guidelines in our
    Contributing document
  • I have checked to ensure there aren't other open Pull Requests for the
    same update/change

PR review checklist

This PR will be evaluated on the basis of the following checks:

  • The task addresses a valid open problem in single-cell analysis
  • The latest version of master is merged and tested
  • The methods/metrics are imported to __init__.py and were tested in the pipeline
  • Method and metric decorators are annotated with paper title, year, author, code
    version, and date
  • The README gives an outline of the methods, metrics and datasets in the folder
  • The README provides a satisfactory task explanation (for new tasks)
  • The sample test data is appropriate to test implementation of all methods and
    metrics (for new tasks)

@mxposed
mxposed marked this pull request as ready for review August 4, 2022 18:14
@codecov

codecov Bot commented Aug 4, 2022

Copy link
Copy Markdown

Codecov Report

Base: 94.64% // Head: 94.67% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (410fc8d) compared to base (b578c4f).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 410fc8d differs from pull request most recent head 7513d41. Consider uploading reports for the commit 7513d41 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #533      +/-   ##
==========================================
+ Coverage   94.64%   94.67%   +0.03%     
==========================================
  Files         138      139       +1     
  Lines        3438     3458      +20     
  Branches      176      176              
==========================================
+ Hits         3254     3274      +20     
  Misses        123      123              
  Partials       61       61              
Flag Coverage Δ
unittests 94.67% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
openproblems/tasks/label_projection/api.py 100.00% <100.00%> (ø)
...nproblems/tasks/label_projection/methods/seurat.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@scottgigante-immunai scottgigante-immunai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the PR! I made a couple changes so the kwargs can be set from the python method call to be consistent with other methods that take parameters, otherwise LGTM!

@mxposed

mxposed commented Aug 10, 2022

Copy link
Copy Markdown
Collaborator Author

Great, thanks! Is there a benchmark run for this anywhere or test benchmark, where I can verify that these predictions score something?

@scottgigante-immunai

Copy link
Copy Markdown
Collaborator

You can use openproblems-cli test for this, e.g.

cd openproblems
docker run -v /tmp:/tmp -v $(pwd):/usr/src/singlecellopenproblems -it singlecellopenproblems/openproblems-r-extras bash
openproblems-cli test --task label_projection --dataset zebrafish_labels --method seurat --metric f1

and compare the printed score to that of others methods

@scottgigante-immunai

Copy link
Copy Markdown
Collaborator

Let me know when you're happy with the parameter choices and I'll merge (once tests pass). In the long run we might implement a hyperparameter search helper to make this easier.

@LuckyMD LuckyMD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall it looks good.. just a documentation question and a technical consideration regarding a possible failure mode.

Comment thread openproblems/tasks/label_projection/methods/seurat.R Outdated
Comment thread openproblems/tasks/label_projection/methods/seurat.py Outdated
@mxposed

mxposed commented Aug 18, 2022

Copy link
Copy Markdown
Collaborator Author

On zebrafish_labels I get this error:

<error/rlang_error>
Error:
! no method or default for coercing “dgRMatrix” to “dgCMatrix”
---
Backtrace:
    ▆
 1. └─`<fn>`(`<SnglCllE[,26022]>`, n_pcs = 50L)
 2.   ├─SeuratObject::as.Seurat(sce, counts = "X", data = NULL)
 3.   └─Seurat:::as.Seurat.SingleCellExperiment(sce, counts = "X", data = NULL)
 4.     └─SeuratObject::CreateAssayObject(counts = mats$counts)
 5.       ├─SeuratObject::as.sparse(x = counts)
 6.       └─SeuratObject:::as.sparse.Matrix(x = counts)
 7.         └─methods::as(object = x, Class = "dgCMatrix")

Have you run into this? I guess it has to do with different storage formats of dataset objects on the python loading side.
With other datasets, unfortunately, I don't have a place to run them with sufficient resources. It get's killed on my laptop, and there's no docker on our cluster.

@scottgigante-immunai

scottgigante-immunai commented Aug 19, 2022

Copy link
Copy Markdown
Collaborator

@mxposed I just added a commit that should handle this. I might eventually roll it into RFunction -- R is really not designed to handle CSR matrices.

@mxposed

mxposed commented Aug 19, 2022

Copy link
Copy Markdown
Collaborator Author

@scottgigante-immunai Thank you, this fixed this issue. Well, now I'm just getting Killed on this dataset too. Is there any cloud allocation for openproblems where I can try running it? Or I get just try running R code without docker

@scottgigante-immunai

Copy link
Copy Markdown
Collaborator

@mxposed your AWS credentials should allow you to spin up an EC2 instance with the AWS CLI. Goes without saying, but please be cognisant of resources and shut down the machine when you are done :)

@scottgigante-immunai

Copy link
Copy Markdown
Collaborator

If you create an instance with aws ec2 run-instances --count 1 --image-id ami-01219569b1bbf9fb2 --instance-type r4.4xlarge. Let me know if you have a permissions error there.

@scottgigante-immunai

Copy link
Copy Markdown
Collaborator

Update: there's some tricky permissions issues here, I'll investigate and give you working commands

@scottgigante-immunai

Copy link
Copy Markdown
Collaborator

Instructions:

aws ec2 create-key-pair --key-name KEY_NAME --key-format pem --query "KeyMaterial" --output text > KEY_NAME.pem
chmod 400 KEY_NAME.pem
aws ec2 run-instances --count 1 --image-id ami-01219569b1bbf9fb2 --instance-type r4.4xlarge --key-name KEY_NAME --security-group-ids sg-01adb52b6d765d80f | grep InstanceId
# wait for boot
aws ec2 describe-instances --instance-id INSTANCE_ID | grep PublicDnsName
ssh -i KEY_NAME.pem ubuntu@PUBLIC_DNS_NAME

@LuckyMD
LuckyMD self-requested a review August 24, 2022 18:03
@scottgigante-immunai

Copy link
Copy Markdown
Collaborator

Unfortunately, the problem with #541 occurs here too. I'll apply the same fix and then I think we should merge.

@mxposed

mxposed commented Aug 25, 2022

Copy link
Copy Markdown
Collaborator Author

Thank you! Maybe it'll fix my testing in Docker on local too. Please wait for me to run the benchmarks.
@scottgigante-immunai I'll try EC2 soon, but I expect running into something, when would you have time to chat about this?

Comment thread openproblems/tasks/label_projection/methods/seurat_wrapper.R
@scottgigante-immunai

Copy link
Copy Markdown
Collaborator

Thank you! Maybe it'll fix my testing in Docker on local too. Please wait for me to run the benchmarks. @scottgigante-immunai I'll try EC2 soon, but I expect running into something, when would you have time to chat about this?

@mxposed drop me a message on CZI biohub slack and we can find some time

@mxposed

mxposed commented Sep 16, 2022

Copy link
Copy Markdown
Collaborator Author

Sorry for delay.

Thank you for EC2 instructions, they work. I have tested seurat method with 4 datasets, I get either 0 or 1, which is very suspicious.
I also get this warning

/usr/local/lib/python3.8/site-packages/anndata/_core/anndata.py:1828: UserWarning: Observation names are not unique. To make them unique, call `.obs_names_make_unique`.

I checked the saved rds object in /tmp and I see there a mix of correct and incorrect predictions in labels_pred. I suspect something is off with either r2py conversion of single-cell-experiment or the metric code post this conversion. Will investigate later.

PS. Seurat methods run in 10-15 mins on EC2. I suspect they should also run on the laptop.

@scottgigante-immunai

Copy link
Copy Markdown
Collaborator

I found the problem. R is implicitly converting adata.obs['is_train'] from bool to int, which means the metrics are not subsetting to the test data correctly.

@mxposed

mxposed commented Oct 5, 2022

Copy link
Copy Markdown
Collaborator Author

Thank you! Sorry I didn't get to this earlier than you!

@scottgigante-immunai

Copy link
Copy Markdown
Collaborator

Performance is now f1=0.958 on pancreas_batch and 0.41 on zebrafish_labels, making this the top performing method so far. Great work!

@scottgigante-immunai
scottgigante-immunai merged commit 3f19f0e into openproblems-bio:main Oct 6, 2022
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.

3 participants