Test Spherize more extensively

cytomining / pycytominer

Python package for processing image-based profiling data

https://pycytominer.readthedocs.io

BSD 3-Clause "New" or "Revised" License

76 stars 34 forks source link

Test Spherize more extensively #127

Closed shntnu closed 3 years ago

shntnu commented 3 years ago

Spherize only tests for the default epsilon

https://github.com/cytomining/pycytominer/blob/dfd355d748f80c658a85720e9c9bfde7ce58c02c/pycytominer/tests/test_operations/test_transform.py#L21-L40

shntnu commented 3 years ago

Hm – actually I don't think the test is correct https://colab.research.google.com/drive/1KitZF-CgV_xgZpd1n0BieQpko-uD4ZQA?usp=sharing

gwaybio commented 3 years ago

i don't have any idea what that collab notebook means!

@shntnu - this is quickly becoming a pretty critical piece in the lincs cell painting dataset. I'd like to solve it once and for all. see broadinstitute/lincs-cell-painting#60.

Does the notebook mean the test needs to be fixed? Or that the test is showing that the spherize function is incorrect in some way? Does the notebook propose a new test?

gwaybio commented 3 years ago

In #132, I added a spherize_epsilon parameter. When we close this issue, we can address this variable here as well.

shntnu commented 3 years ago

Sorry for leaving this stray comment without an explanation :D

The notebook shows that this (covariance) matrix will pass the test even though it is not spherized.

But as we discussed, you needn't do more here. The code looks right!

shntnu commented 3 years ago

Oh I just noticed this https://github.com/cytomining/pycytominer/issues/127#issuecomment-801467132

nvm – feel free to reopen or use a new issue, as you see fit @gwaygenomics