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

Skip cell annotation if cell annotation sources empty #3

Merged
merged 9 commits into from
Oct 5, 2023

Conversation

vucinick
Copy link
Contributor

Fixes issue #2

Description of the fix

This fix addresses the issue where declaring CELL_ANNOTATION_SOURCES: null in the 02_norm_clustering.yaml configuration file would result in the pipeline failing. The reason was that it creates an empty list which isn't null.

The fix now checks the length of the list, and if it's 0, it skips the cell annotation step, ensuring the expected behavior mentioned in the issue. Additionally, I've updated the relevant Rmd files to determine whether to include the cell annotation part in the report.

I've also made an addition to the vignette to document that there is an option to use null for cell annotation sources. This can be particularly useful for cases involving non-model organisms with limited annotation sources.

Please review and merge this pull request to resolve the issue. Thank you!

@gorgitko
Copy link
Contributor

Hey @vucinick, thank you for the PR.

While your solution is valid, I think easier would be to remove these lines

if (is_empty(cell_annotation_sources)) {
return(list())
}

and CELL_ANNOTATION_SOURCES will stay NULL after the config load.

I think the reason why this param is changed to an empty list is that the cell annotation subplan was earlier

cell_annotation = target(
cell_annotation_fn(cell_annotation_params, sce_rm_doublets, BPPARAM = ignore(BiocParallel::bpparam())),
dynamic = map(cell_annotation_params)
),

not separated from the main plan, but there couldn't be just NULL, because drake is then not able to dynamic map over it (cell_annotation_params).


It's currently not documented, but you can run unit and end-to-end tests through https://github.com/bioinfocz/scdrake/blob/main/dev/run_tests.R:

docker exec -it -u rstudio -w /home/rstudio/scdrake_source <container> \
  r --interactive -L /usr/local/lib/R/site-library -t dev/run_tests.R \
  --output-dir /home/rstudio/shared/scdrake_run_tests \
  --output-dir-pipeline-tests /home/rstudio/shared/scdrake_run_tests/pipeline_outputs

It assumes your scdrake local repo is mounted as /home/rstudio/scdrake_source

Check Rscript dev/run_tests.R -h for more options.

@gorgitko gorgitko self-requested a review September 18, 2023 06:19
vignettes/stage_norm_clustering.Rmd Outdated Show resolved Hide resolved
@vucinick
Copy link
Contributor Author

Hi @gorgitko,
I managed to fix the issue with your proposed solution with a modification. I am unsure if I correctly interpreted the pipeline, but I had to remove creation of an empty list in two files:

if (is_empty(cell_annotation_sources)) {
return(list())
}

and
if (is_empty(cell_annotation_sources)) {
return(list(NA))
}

The empty file creation in R/cell_annotation.R seemed unnecessary with my solution, so I remove it in cell_annotation_params_fn.

The main part would be in R/config_process_single_sample.R, where I added the part to checks if the CELL_ANNOTATION_SOURCES is null or not. Depending on this, it either skips or continues with preparing the cell_annotation_sources_params.
As I am writing this, I realized I missed this part in R/config_process_integration.R. I will add it later, if this solution is correct.

I ran the test for the single_sample pipeline, and it correctly creates normalization reports without the cell annotation 😁.

@gorgitko
Copy link
Contributor

gorgitko commented Oct 1, 2023

Hi @vucinick, looks good! Please, add the relevant part also to the R/config_process_integration.R. Thanks!

@gorgitko gorgitko force-pushed the skip_cell_annotation branch from 8e3f03f to b39903e Compare October 5, 2023 09:37
@gorgitko gorgitko merged commit b689bfe into main Oct 5, 2023
1 check passed
@gorgitko gorgitko deleted the skip_cell_annotation branch October 5, 2023 10:06
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