Prototype DE pipeline for dense matrix inputs (SCP-4074) #238

jlchang · 2022-03-24T13:50:48Z

To allow progress on engineering-side development work for the differential expression (DE) feature, this PR aims to provide a working "happy path" DE process that generates output files from a DE analysis on a couple of known good datasets (SCP1671 and SCP1539; Note, the test case is derived from SCP1677 - the cell_type assignments in the de_integration_metadata.tsv file are bogus)

This prototype pipeline still needs tests (both integration and unit tests), try/except clauses etc. and capability to run on studies that have sparse matrix. Suggestions welcomed for how best to harden this code - please indicate whether the suggestion should be implemented for this PR or can be incorporated into a downstream "hardening" PR.

To test:

activate the scp-ingest-pipeline repo virtualenv (added scanpy, updated numpy and scipy versions)
then from the ingest directory of the scp-ingest-pipeline repo:

source ../scripts/setup_mongo_dev.sh <path to your Github token file>
unset BARD_HOST_URL

python ingest_pipeline.py --study-id addedfeed000000000000000 --study-file-id dec0dedfeed1111111111111 differential_expression --annotation cell_type__ontology_label --matrix-file-path ../tests/data/differential_expression/de_integration.tsv --matrix-file-type dense --cell-metadata-file ../tests/data/differential_expression/de_integration_unordered_metadata.tsv --cluster-file ../tests/data/differential_expression/de_integration_cluster.tsv --name de_integration --study-accession SCPdev --differential_expression

confirm that the script runs successfully and generates the following files:

de_integration--cell_type__ontology_label--cholinergic_neuron--wilcoxon.tsv
de_integration--cell_type__ontology_label--cranial_somatomotor_neuron--wilcoxon.tsv
de_integration--cell_type__ontology_label--pyramidal_neuron--wilcoxon.tsv
de_integration--cell_type__ontology_label--somatomotor_neuron--wilcoxon.tsv
de_integration--cell_type__ontology_label--sympathetic_cholinergic_neuron--wilcoxon.tsv
de_log.txt

Content of:

de_integration--cell_type__ontology_label--cholinergic_neuron--wilcoxon.tsv

can be compared to a reference file at ../tests/data/differential_expression/reference

This PR supports SCP-4074.

remove spaces from filename reduce significant digits log exception when annot missing

jlchang · 2022-03-24T14:31:40Z

Had CI failure for test_make_toy with ConnectionResetError: [Errno 104] Connection reset by peer
Retried CI with success - false positive was likely transient failure with 3rd party dependency.

codecov · 2022-03-24T14:33:35Z

Codecov Report

Merging #238 (5928217) into development (6fff171) will decrease coverage by 1.61%.
The diff coverage is 31.61%.

@@               Coverage Diff               @@
##           development     #238      +/-   ##
===============================================
- Coverage        72.90%   71.29%   -1.62%     
===============================================
  Files               26       27       +1     
  Lines             3385     3518     +133     
===============================================
+ Hits              2468     2508      +40     
- Misses             917     1010      +93

Impacted Files	Coverage Δ
ingest/de.py	`22.01% <22.01%> (ø)`
ingest/ingest_pipeline.py	`61.29% <38.46%> (-1.40%)`	⬇️
ingest/annotations.py	`89.50% <100.00%> (+0.05%)`	⬆️
ingest/cli_parser.py	`76.59% <100.00%> (+3.42%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fff171...5928217. Read the comment docs.

eweitz

Kudos, I'm really excited for this prototype! A solid start to a big new scientific feature.

I note a trivial CLI UI blocker, and a bunch of non-blocking suggestions.

eweitz · 2022-03-24T16:46:22Z

ingest/de.py

+        cluster_cell_list = []
+        for value in cluster_cell_values:
+            cluster_cell_list.extend(value)


Could this be simplified / optimized like so?

Suggested change

cluster_cell_list = []

for value in cluster_cell_values:

cluster_cell_list.extend(value)

cluster_cell_list = []

if len(cluster_cell_values) > 0:

cluster_cell_list = cluster_cell_values

cluster_cells.tolist() yields a list of (single value) lists that needs to be flattened
using extend converts each single-value list (ie. cell name) to a plain value

I'll add a comment that reflects the above complication.

eweitz · 2022-03-24T16:55:36Z

ingest/de.py

+                self.genes,
+                self.barcodes,
+            )
+            DifferentialExpression.de_logger.info(f"preparing DE on sparse matrix")


No f-string needed here:

Suggested change

DifferentialExpression.de_logger.info(f"preparing DE on sparse matrix")

DifferentialExpression.de_logger.info("preparing DE on sparse matrix")

eweitz · 2022-03-24T16:55:50Z

ingest/de.py

+                self.cluster_name,
+                self.method,
+            )
+            DifferentialExpression.de_logger.info(f"preparing DE on dense matrix")


No f-string needed here:

Suggested change

DifferentialExpression.de_logger.info(f"preparing DE on dense matrix")

DifferentialExpression.de_logger.info("preparing DE on dense matrix")

eweitz · 2022-03-24T16:56:38Z

ingest/de.py

+        """
+        """


Could you add a description here? Otherwise such stragglers seems best to omit.

removed 5928217

eweitz · 2022-03-24T16:57:13Z

ingest/de.py

+            # For AnnData, obs are cells and vars are genes
+            # BUT transpose needed for both dense and sparse
+            # so transpose step is after this data object composition step
+            # therefore the assignements below are the reverse of expected


Helpful note!

eweitz · 2022-03-24T17:01:52Z

ingest/de.py

+                f'{cluster_name}--{annotation}--{str(group_filename)}--{method}.tsv'
+            )
+
+            rank.to_csv(out_file, sep='\t', float_format='%.4g', index=False)


This rounds instead of crudely truncating, right? In either case, a brief explanatory note would ease understanding.

Suggested change

rank.to_csv(out_file, sep='\t', float_format='%.4g', index=False)

# Round numbers to 4th decimal place, e.g. 0.12345 -> 0.1235

rank.to_csv(out_file, sep='\t', float_format='%.4g', index=False)

eweitz · 2022-03-24T17:04:44Z

ingest/ingest_pipeline.py

+            **self.kwargs,
+        )
+        de.execute_de()
+        # ToDo: surface failed DE for analytics


Could you ticket this? We typically do that for all TODOs.

Suggested change

# ToDo: surface failed DE for analytics

# ToDo: surface failed DE for analytics

Yup, created SCP-4206 and noted in comment. 5928217

eweitz · 2022-03-24T17:13:34Z

ingest/ingest_pipeline.py

+    elif "differential_expression" in arguments:
+        if arguments["differential_expression"]:
+            config.set_parent_event_name(
+                "ingest-pipeline:differential_expression:ingest"
+            )
+            status_de = ingest.calculate_de()
+            status.append(status_de)


The inner condition strikes me as something that ought to be redundant with the outer condition.

Having ingest as the terminal segment of the event name also feels confusing. In the context of our pipelines, it'd be cool if we could simply say "ingest" to mean "traditionally transform and load SCP study files to MongoDB" and "differential expression" to mean "run DE calculations on previously-validated and ingested data".

Finally, trivially, delimiter should be hyphen instead of underscore.

Suggested change

elif "differential_expression" in arguments:

if arguments["differential_expression"]:

config.set_parent_event_name(

"ingest-pipeline:differential_expression:ingest"

)

status_de = ingest.calculate_de()

status.append(status_de)

elif "differential-expression" in arguments:

config.set_parent_event_name(

"ingest-pipeline:differential-expression"

)

status_de = ingest.calculate_de()

status.append(status_de)

eweitz · 2022-03-24T17:24:02Z

ingest/cli_parser.py

+    )
+
+    parser_differential_expression.add_argument(
+        "--differential_expression",


Blocker: for command-line arguments here, we always delimit with hyphens.

Suggested change

"--differential_expression",

"--differential-expression",

eweitz · 2022-03-24T17:27:03Z

ingest/ingest_pipeline.py

    from .expression_files.dense_ingestor import DenseIngestor
    from .expression_files.mtx import MTXIngestor
    from .cli_parser import create_parser, validate_arguments
+    from .de import DifferentialExpression, prepare_h5ad


This looks unused in this module, so let's remove it.

Otherwise, I think we'd need to also include it in the import block above this one.

Suggested change

from .de import DifferentialExpression, prepare_h5ad

from .de import DifferentialExpression

Thanks for catching that! I had removed from the top import block and neglected the second. 5928217

eweitz

(I meant to request changes for that trivial blocker per my previous review summary.)

address PR comments

devonbush

Approved for merge -- we'll do a deep code dive later

jlchang added 13 commits March 4, 2022 17:36

add DE option to cli_parser

994506a

update libraries for scanpy

5d82396

partial DE analysis process

e18b62e

coerce group-annotations earlier in workflow

220a080

working code for testing SCP1671 files locally

b032d2d

create methods for DifferentialExpression Class

32e5b18

make DE staticmethods more easily testable

5a039ac

added cluster to output filename; log DE steps

1b95579

ensure metadata cell order before joining

2246974

remove spaces from filename reduce significant digits log exception when annot missing

fix whitespace issue with get_cell_names

2ec43a4

more unit testable functions

45377bc

inputs for de integration test

654b0c1

add reference file for de integration

cfa9129

jlchang requested review from devonbush, ehanna4 and eweitz March 24, 2022 13:50

eweitz approved these changes Mar 24, 2022

View reviewed changes

eweitz requested changes Mar 24, 2022

View reviewed changes

restore rank info to DE output files

5928217

address PR comments

jlchang requested a review from eweitz March 25, 2022 19:00

eweitz approved these changes Mar 25, 2022

View reviewed changes

devonbush approved these changes Mar 28, 2022

View reviewed changes

jlchang merged commit 90db079 into development Mar 28, 2022

jlchang deleted the jlc_make_data_objects branch March 28, 2022 15:07

jlchang mentioned this pull request Mar 29, 2022

Update to Ingest Pipeline 1.15.0 broadinstitute/single_cell_portal_core#1434

Merged

	DifferentialExpression.de_logger.info(f"preparing DE on sparse matrix")
	DifferentialExpression.de_logger.info("preparing DE on sparse matrix")

	rank.to_csv(out_file, sep='\t', float_format='%.4g', index=False)
	# Round numbers to 4th decimal place, e.g. 0.12345 -> 0.1235
	rank.to_csv(out_file, sep='\t', float_format='%.4g', index=False)

	# ToDo: surface failed DE for analytics
	# ToDo: surface failed DE for analytics

	from .de import DifferentialExpression, prepare_h5ad
	from .de import DifferentialExpression

Prototype DE pipeline for dense matrix inputs (SCP-4074) #238

Prototype DE pipeline for dense matrix inputs (SCP-4074) #238

Uh oh!

Conversation

jlchang commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlchang commented Mar 24, 2022

Uh oh!

codecov bot commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eweitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eweitz left a comment

Choose a reason for hiding this comment

Uh oh!

devonbush left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jlchang commented Mar 24, 2022 •

edited

Loading

codecov bot commented Mar 24, 2022 •

edited

Loading