Intake lists #52

michaelbornholdt · 2021-04-26T20:19:50Z

Enrichment and Precision recall shall intake a list of variables. This way the similarity matrix is only computed once!

Update the Fork

codecov-commenter · 2021-04-27T13:21:20Z

Codecov Report

Merging #52 (5fb4d24) into master (220b296) will decrease coverage by 0.09%.
The diff coverage is 97.29%.

❗ Current head 5fb4d24 differs from pull request most recent head 04be210. Consider uploading reports for the commit 04be210 to get more accurate results

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
- Coverage   98.36%   98.26%   -0.10%     
==========================================
  Files          24       24              
  Lines         855      865      +10     
==========================================
+ Hits          841      850       +9     
- Misses         14       15       +1

Flag	Coverage Δ
unittests	`98.26% <97.29%> (-0.10%)`	⬇️

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

Impacted Files	Coverage Δ
cytominer_eval/evaluate.py	`100.00% <ø> (ø)`
cytominer_eval/tests/test_evaluate.py	`100.00% <ø> (ø)`
cytominer_eval/operations/enrichment.py	`95.45% <93.33%> (-4.55%)`	⬇️
cytominer_eval/operations/precision_recall.py	`100.00% <100.00%> (ø)`
...iner_eval/tests/test_operations/test_enrichment.py	`100.00% <100.00%> (ø)`
...val/tests/test_operations/test_precision_recall.py	`100.00% <100.00%> (ø)`
cytominer_eval/transform/util.py	`100.00% <100.00%> (ø)`

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 220b296...04be210. Read the comment docs.

michaelbornholdt · 2021-04-27T13:25:49Z

@gwaygenomics This last Commit should fix every thing. So this is ready for review :)

michaelbornholdt · 2021-04-27T13:26:33Z

Fixes #51

michaelbornholdt · 2021-04-27T13:27:28Z

Ah, I still need to run the Demo notebook

michaelbornholdt · 2021-04-27T15:19:52Z

@gwaygenomics I reran the demo, All up to do date now

gwaybio

Very nice work. One main discussion point: Do we want to force the user to input a list, or should we allow single elements as well?

A supplementary point is to please add back the Cell Painting demo notebook (I think you accidentally deleted it)

cytominer_eval/evaluate.py

gwaybio · 2021-04-28T13:51:34Z

cytominer_eval/evaluate.py

    grit_control_perts: List[str] = ["None"],
    grit_replicate_summary_method: str = "mean",
    mp_value_params: dict = {},
-    enrichment_percentile: float = 0.5,
+    enrichment_percentile: List[float] = [0.99, 0.98],


Same comment about accepting both single values and lists - do you agree?

Also, nice to see that you're suggesting a more reasonable default after your experimentation!

gwaybio · 2021-04-28T13:52:07Z

cytominer_eval/evaluate.py

@@ -129,7 +130,7 @@ def evaluate(
        metric_result = precision_recall(
            similarity_melted_df=similarity_melted_df,
            replicate_groups=replicate_groups,
-            k=precision_recall_k,
+            k_list=precision_recall_k,


will need to revisit this decision pending discussion on accepting both ints and list of ints.

gwaybio · 2021-04-28T13:52:50Z

cytominer_eval/operations/enrichment.py

-) -> dict:
+    similarity_melted_df: pd.DataFrame,
+    replicate_groups: List[str],
+    percentile: List[float],


just making sure that you're intentionally removing the default of 0.9.

Also, this will need to change depending on how we decide users should interact (single int or list of ints)

gwaybio · 2021-04-28T13:53:38Z

cytominer_eval/operations/enrichment.py

@@ -28,48 +30,54 @@ def enrichment(
    replicate_groups : List
        a list of metadata column names in the original profile dataframe to use as
        replicate columns.
-    percentile :  float
+    enrichment_percentile :  List of floats


same comment about might need to change.

also, shouldn't this just be percentile instead of enrichment_percentile? If not, then u need to change the function argument name

enrichment_percentile is still not the name of the function argument (line 19 above) do you see what I mean?

I thinks it fine how it is now. Do you think I should change the name of the variable in enrichment.py to enrichment_percentile instead of just percentile?

cytominer_eval/operations/enrichment.py

gwaybio · 2021-04-28T13:58:33Z

cytominer_eval/operations/precision_recall.py

-    similarity_melted_df: pd.DataFrame,
-    replicate_groups: List[str],
-    k: int,
+    similarity_melted_df: pd.DataFrame, replicate_groups: List[str], k_list: List[int],


same comment about int vs. list[int] decisions.

Also, I LOVE the fact that you're making this enhancement all in one shot, and not just to the enrichment metric. So useful to have all these changes in one PR.

gwaybio · 2021-04-28T13:59:37Z

cytominer_eval/tests/test_operations/test_enrichment.py

-    assert result_df.shape == (7, 4)
-    assert result_df.percentile[0] == 1.0
+    assert result.shape == (7, 4)
+    assert result.enrichment_percentile[0] == 1.0


can you add a check to the second element in this Series?

not sure what you mean

assert that result.enrichment_percentile[1] equals what you'd calculate by hand.

cytominer_eval/tests/test_operations/test_enrichment.py

Co-authored-by: Greg Way <gregory.way@gmail.com>

michaelbornholdt · 2021-04-30T15:47:56Z

@gwaygenomics should be done with all your comments now

gwaybio

only minor comments, but they should be addressed before merge.

cytominer_eval/evaluate.py

cytominer_eval/operations/enrichment.py

gwaybio · 2021-04-30T19:27:54Z

cytominer_eval/operations/enrichment.py

@@ -28,48 +30,54 @@ def enrichment(
    replicate_groups : List
        a list of metadata column names in the original profile dataframe to use as
        replicate columns.
-    percentile :  float
+    enrichment_percentile :  List of floats


enrichment_percentile is still not the name of the function argument (line 19 above) do you see what I mean?

gwaybio · 2021-04-30T19:29:57Z

cytominer_eval/tests/test_evaluate.py

@@ -160,7 +156,7 @@ def test_evaluate_precision_recall():
            replicate_groups=["Metadata_broad_sample"],
            operation="precision_recall",
            similarity_metric="pearson",
-            precision_recall_k=k,
+            precision_recall_k=[k],


can you try one of these without a list? (i mean one of either lines 144 or 163)

It should work either way

Can you add a quick comment on line 151 noting the difference between evaluate on line 152 and evaluate on line 137?

For future changes to this code, it will be important to note that we're also testing the list/int capability of this argument

gwaybio · 2021-04-30T19:30:49Z

cytominer_eval/tests/test_operations/test_enrichment.py

-    assert result_df.shape == (7, 4)
-    assert result_df.percentile[0] == 1.0
+    assert result.shape == (7, 4)
+    assert result.enrichment_percentile[0] == 1.0


assert that result.enrichment_percentile[1] equals what you'd calculate by hand.

gwaybio · 2021-04-30T19:32:24Z

cytominer_eval/tests/test_operations/test_precision_recall.py

@@ -42,11 +42,11 @@ def test_precision_recall():
    result = precision_recall(
        similarity_melted_df=similarity_melted_df,
        replicate_groups=replicate_groups,
-        k=10,
+        k=[5, 10],


can you also keep a test for k as an int?

This test is good, but maybe add a test asserting that the outputs are the same for k=10 and k=[10]

Co-authored-by: Greg Way <gregory.way@gmail.com>

michaelbornholdt · 2021-05-04T18:06:48Z

@gwaygenomics ready for you to look at again :)

gwaybio

Very minor comments, I'll merge after they are addressed!

cytominer_eval/evaluate.py

gwaybio · 2021-05-04T18:30:40Z

cytominer_eval/tests/test_evaluate.py

@@ -160,7 +156,7 @@ def test_evaluate_precision_recall():
            replicate_groups=["Metadata_broad_sample"],
            operation="precision_recall",
            similarity_metric="pearson",
-            precision_recall_k=k,
+            precision_recall_k=[k],


Can you add a quick comment on line 151 noting the difference between evaluate on line 152 and evaluate on line 137?

For future changes to this code, it will be important to note that we're also testing the list/int capability of this argument

cytominer_eval/tests/test_operations/test_enrichment.py

Co-authored-by: Greg Way <gregory.way@gmail.com>

michaelbornholdt · 2021-05-06T18:53:34Z

@gwaygenomics back to you again :)

michaelbornholdt · 2021-05-06T18:54:13Z

What is this change request, how do I get rid of it?

gwaybio · 2021-05-06T19:05:14Z

No worries, I get rid of it when I finally approve. Looking now

gwaybio · 2021-05-06T19:09:21Z

Wonderfully done - merging now

michaelbornholdt and others added 4 commits April 26, 2021 11:42

Merge pull request #1 from cytomining/master

cc6331e

Update the Fork

Add list input to enrichment.py

dda345e

delete prints

470ef91

Apply Black

a0f7030

michaelbornholdt changed the title ~~Intake lists~~ Draft: Intake lists Apr 26, 2021

michaelbornholdt added 4 commits April 26, 2021 16:55

modify test two new input

9d0eb6f

Change the input of precision_recall.py

4b123b6

Black changes

1584d13

fix tests

d732dce

michaelbornholdt changed the title ~~Draft: Intake lists~~ Intake lists Apr 27, 2021

rerun the demo

86e5314

gwaybio self-requested a review April 28, 2021 13:47

gwaybio requested changes Apr 28, 2021

View reviewed changes

michaelbornholdt and others added 5 commits April 30, 2021 11:08

also accept ints

a908730

Co-authored-by: Greg Way <gregory.way@gmail.com>

ints and floats also allowed

03cb138

add further tests

2b38adb

Black

d6a24f4

fix test

60d8868

michaelbornholdt added 2 commits April 30, 2021 11:48

Add Demo

2f631da

Fix test

25f58bc

gwaybio self-requested a review April 30, 2021 19:24

gwaybio requested changes Apr 30, 2021

View reviewed changes

michaelbornholdt and others added 2 commits May 4, 2021 11:27

change input to floats

6d42cd2

Co-authored-by: Greg Way <gregory.way@gmail.com>

correct doc

bbdfe74

michaelbornholdt and others added 5 commits May 4, 2021 11:29

Merge remote-tracking branch 'origin/intake_lists' into intake_lists

06d46aa

change input to floats

fc15e74

Co-authored-by: Greg Way <gregory.way@gmail.com>

Merge remote-tracking branch 'origin/intake_lists' into intake_lists

ad3754d

More tests

c353179

named percentile in enrichment.py

9123f51

gwaybio reviewed May 4, 2021

View reviewed changes

michaelbornholdt and others added 4 commits May 6, 2021 14:44

update docstring

5fb4d24

Co-authored-by: Greg Way <gregory.way@gmail.com>

Update cytominer_eval/tests/test_operations/test_enrichment.py

bc679fe

Co-authored-by: Greg Way <gregory.way@gmail.com>

add comment for test

990e14e

finalize test enrichment

04be210

michaelbornholdt closed this May 6, 2021

michaelbornholdt reopened this May 6, 2021

gwaybio approved these changes May 6, 2021

View reviewed changes

gwaybio merged commit 7f94b11 into cytomining:master May 6, 2021

Intake lists #52

Intake lists #52

Conversation

michaelbornholdt commented Apr 26, 2021

codecov-commenter commented Apr 27, 2021 • edited Loading

Codecov Report

michaelbornholdt commented Apr 27, 2021

michaelbornholdt commented Apr 27, 2021

michaelbornholdt commented Apr 27, 2021

michaelbornholdt commented Apr 27, 2021

gwaybio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelbornholdt commented Apr 30, 2021

gwaybio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelbornholdt commented May 4, 2021

gwaybio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelbornholdt commented May 6, 2021

michaelbornholdt commented May 6, 2021

gwaybio commented May 6, 2021

gwaybio commented May 6, 2021

codecov-commenter commented Apr 27, 2021 •

edited

Loading