koalive commented 3 years ago

Hi! Here is the PR to add the mp-value computation per perturbation (replicates are merged as described in the original paper). I tried to stay as close as possible to the original definition for now but a lot more can be tweaked if needed. Hopefully this integrates correctly with the other operations but I could actually not reuse so many of the utils methods as the mp-value is not correlation-based but relies on the Mahalanobis distance. Concretely this means similarity_melted_df is not needed for the computation of the mp-value.

Limitations:

This might still need some linting
In cytominer_eval.evaluate it would be possible to skip the computation of the melted data frame when operation == "mp_value"
I do most of my testing in Jupyter notebooks so I hope I didn't add mistakes when migrating the code to the corresponding files.
In the methods section of the paper they state "To find S, the covariance matrices are first calculated separately for the treatment and vehicle subsets of P, and then each resulting covariance matrix is weighted by the fraction of total samples in P that it contains. The weighted matrices are summed to produce S." which I did not do for now. This should not affect the result too much anyway.
The Mahalanobis distance is implemented in cytominer_eval.operations.util.calculate_mahalanobis. This could be implemented as separate operation (either in this PR or in another one). I don't think this would make sense to implement directly as a metric as this is not design for pairwise distances.

Cheers.

gwaybio commented 3 years ago

@koalive - I literally just saw this now! Somehow this notification got mixed up somewhere. I will review ASAP. Thank you

gwaybio commented 3 years ago

One quick thing as I review the remainder of the files:

This might still need some linting

Can you run black and then add the updated files? This automates the linting process and all other scripts have been adjusted

koalive commented 3 years ago

Hi Greg, Happy new year to you too! No worries, thanks for reviewing my PR. With these new commits, I think I addressed your comments and I'll reply to some of them with more details. Overall, regarding the interpretability of these values in comparison to distances, maybe we could simply use the -log10 of the mp-value? Let me know what you think!

codecov-io commented 3 years ago

Codecov Report

Merging #26 (3669659) into master (6f9d350) will increase coverage by 0.09%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
+ Coverage   99.35%   99.44%   +0.09%     
==========================================
  Files          19       21       +2     
  Lines         616      726     +110     
==========================================
+ Hits          612      722     +110     
  Misses          4        4

Flag	Coverage Δ
unittests	`99.44% <100.00%> (+0.09%)`	:arrow_up:

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

Impacted Files	Coverage Δ
cytominer_eval/operations/precision_recall.py	`100.00% <ø> (ø)`
cytominer_eval/transform/transform.py	`97.14% <ø> (ø)`
cytominer_eval/evaluate.py	`100.00% <100.00%> (ø)`
cytominer_eval/operations/__init__.py	`100.00% <100.00%> (ø)`
cytominer_eval/operations/mp_value.py	`100.00% <100.00%> (ø)`
cytominer_eval/operations/util.py	`100.00% <100.00%> (ø)`
cytominer_eval/tests/test_evaluate.py	`100.00% <100.00%> (ø)`
...ominer_eval/tests/test_operations/test_mp_value.py	`100.00% <100.00%> (ø)`
...ominer_eval/tests/test_transform/test_transform.py	`100.00% <100.00%> (ø)`
cytominer_eval/tests/test_transform/test_util.py	`100.00% <100.00%> (ø)`
... and 3 more

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 6f9d350...3669659. Read the comment docs.

koalive commented 3 years ago

Good catch, I added the parametrization of the mp-value computations at the mp-value and evaluate stages as well and tested for incorrect parameter names in all cases. If I'm not missing anything, this should be ready for merging! 👍

gwaybio commented 3 years ago

I'm going to merge. Thanks again @koalive!! I am eager to collaborate in the future, and will keep you posted about manuscript plans

cytomining / cytominer-eval

Add mp-values #26

Codecov Report