Refactor utility functions #55

gwaybio · 2021-05-11T21:18:10Z

The utility functions were getting clunky. I went to add new functionality to grit, but I struggled to figure out exactly where I should be adding it. This led me to this refactoring exercise.

No functionality was changed in this pull request. Only names and locations of python utility functions and tests. I also needed to modify several additional functions to fix import statements.

I also made super minor changes to formatting and ~~fixed a deprecated testing warning~~ (reverted back, see #56 ).

Note, I did fix a separate deprecation warning for numpy built-in types (see https://numpy.org/doc/stable/release/1.20.0-notes.html#deprecations)

gwaybio · 2021-05-11T21:20:57Z

cc @michaelbornholdt - I just gave you triage access to the repo, so that you are able to more effectively review this PR. Once you accept the invite, I will add you as a reviewer. Thanks! :)

codecov-commenter · 2021-05-12T18:41:59Z

Codecov Report

Merging #55 (03c09ee) into master (7f94b11) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   98.38%   98.45%   +0.06%     
==========================================
  Files          24       30       +6     
  Lines         867      905      +38     
==========================================
+ Hits          853      891      +38     
  Misses         14       14

Flag	Coverage Δ
unittests	`98.45% <100.00%> (+0.06%)`	⬆️

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

Impacted Files	Coverage Δ
...iner_eval/tests/test_operations/test_enrichment.py	`100.00% <ø> (ø)`
cytominer_eval/evaluate.py	`100.00% <100.00%> (ø)`
cytominer_eval/operations/enrichment.py	`100.00% <100.00%> (ø)`
cytominer_eval/operations/grit.py	`100.00% <100.00%> (ø)`
cytominer_eval/operations/mp_value.py	`100.00% <100.00%> (ø)`
cytominer_eval/operations/precision_recall.py	`100.00% <100.00%> (ø)`
...miner_eval/operations/replicate_reproducibility.py	`100.00% <100.00%> (ø)`
cytominer_eval/tests/test_evaluate.py	`100.00% <100.00%> (ø)`
cytominer_eval/tests/test_operations/test_grit.py	`100.00% <100.00%> (ø)`
...ominer_eval/tests/test_operations/test_mp_value.py	`100.00% <100.00%> (ø)`
... and 19 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 7f94b11...03c09ee. Read the comment docs.

gwaybio · 2021-05-12T19:04:45Z

alright @michaelbornholdt - this is ready for your eyes.

This blog post might be helpful to determine how to review.

Personally, I find reviewing other PRs to be helpful for the times when it's me actually proposing the changes. For this reason (and for an independent reason: I think more eyes on code the better) I am requesting your review. Do you think you'll be able to do this in a timely fashion? Hopefully by tomorrow?

FWIW, I think this PR is an easy one to start with. I'm not changing any functionality, just structure.

michaelbornholdt

Overall very clear PR @gwaygenomics . I only have mini suggestions and questions.
I will run Black over all the code because I remember it having some issues and we can use this refactoring to solve those.

michaelbornholdt · 2021-05-18T15:01:32Z

cytominer_eval/evaluate.py

@@ -8,7 +8,7 @@
 from typing import List, Union

 from cytominer_eval.transform import metric_melt
-from cytominer_eval.transform.util import check_replicate_groups
+from cytominer_eval.utils.transform_utils import check_replicate_groups


For me, this line gives an error because utils are not imported here or in the init.py file. But I am unsure why the tests don't give that error?

This applies to all imports of util below

oh interesting, what is the error?

Is it possible that your testing environment is out-of-date?

I also added a __init__.py file inside cytominer_eval/utils - so this might help. It should have been there in the first place, so thanks for making this note.

Yea I know it works, for some reason pycharm is complaining about it. I think it's because if you run that file on its own, it won't find the util.
but all good

michaelbornholdt · 2021-05-18T15:12:50Z

cytominer_eval/tests/test_evaluate.py

@@ -202,7 +208,9 @@ def test_evaluate_grit():
    top_result = (
        grit_results_df.sort_values(by="grit", ascending=False)
        .reset_index(drop=True)
-        .iloc[0,]


Is this black?
it doesn't seem like a helpful change in code to me. Same goes for the lines below.

Yep, it is, just checked it. I do not know why black is only modifying this now...

It is definitely a non-functional change, but it's also harmless. I think we can ignore it

michaelbornholdt · 2021-05-18T15:16:57Z

cytominer_eval/tests/test_operations/test_grit.py

    get_grit_entry,
    calculate_grit,
 )

+


This is something we need to change for some other files, I will point out below where we are getting the two spacing incorrect. This line is correct, but other files are not

nice! Weird that black didn't catch the other one. I wonder if it's a version thing.

These version differences could get annoying... I wondered if there is a way to automate black, and it looks like there is via github actions! https://github.com/psf/black/actions/runs/17913292/workflow

I added #57 for us to track it there.

Thanks for adding the extra changes

cytominer_eval/tests/test_operations/test_mp_value.py

cytominer_eval/tests/test_utils/test_availability_utils.py

michaelbornholdt · 2021-05-18T15:19:38Z

cytominer_eval/tests/test_utils/test_grit_utils.py

+
+from cytominer_eval.utils.availability_utils import get_available_grit_summary_methods
+
+random.seed(123)


To be really honest Greg, I think you should definitely change this to be seed(42) for everything 🤷 :)

hahaha, yes, it is the answer to the universe after all. Feel free to fix in your next PR :)

cytominer_eval/tests/test_utils/test_transform_utils.py

cytominer_eval/utils/grit_utils.py

cytominer_eval/utils/transform_utils.py

michaelbornholdt · 2021-05-18T15:29:13Z

And Black checks out with no errors! NICE

Co-authored-by: Michael Bornholdt <[email protected]>

Fixing spaces after function import, docstring consistency, and a missing numpy builtin update Co-authored-by: Michael Bornholdt <[email protected]>

gwaybio

Thanks for these comments. I've either made replies that require your followup or resolved the issue. I'll re-request your review.

The most important comment is for you to confirm that these updates work on your end. Although that is less important since they do seem to pass tests. Weird!

gwaybio · 2021-05-18T17:40:05Z

cytominer_eval/tests/test_evaluate.py

@@ -202,7 +208,9 @@ def test_evaluate_grit():
    top_result = (
        grit_results_df.sort_values(by="grit", ascending=False)
        .reset_index(drop=True)
-        .iloc[0,]


Yep, it is, just checked it. I do not know why black is only modifying this now...

It is definitely a non-functional change, but it's also harmless. I think we can ignore it

gwaybio · 2021-05-18T17:48:28Z

cytominer_eval/tests/test_operations/test_grit.py

    get_grit_entry,
    calculate_grit,
 )

+


nice! Weird that black didn't catch the other one. I wonder if it's a version thing.

These version differences could get annoying... I wondered if there is a way to automate black, and it looks like there is via github actions! https://github.com/psf/black/actions/runs/17913292/workflow

I added #57 for us to track it there.

Thanks for adding the extra changes

cytominer_eval/utils/transform_utils.py

gwaybio · 2021-05-18T17:52:56Z

cytominer_eval/tests/test_utils/test_grit_utils.py

+
+from cytominer_eval.utils.availability_utils import get_available_grit_summary_methods
+
+random.seed(123)


hahaha, yes, it is the answer to the universe after all. Feel free to fix in your next PR :)

michaelbornholdt

I'm fine with all of this.
Yes, should definitely set the exact config and version for black.
Ideally, you would also want to use pipenv instead of requirements.txt

You can merge yourself :)

michaelbornholdt · 2021-05-18T18:08:58Z

@gwaygenomics

gwaybio · 2021-05-18T18:13:07Z

Awesome, I will merge now. Thanks!

Ideally, you would also want to use pipenv instead of requirements.txt

I am interested. Can you elaborate? Perhaps you can elaborate in a new github issue so we can possibly implement this enhancement in the future?

gwaybio added 7 commits May 11, 2021 16:13

move operations and transform util files into a dedicated utils folder

b556c41

move testing util files to a new test_utils folder

de4a799

move assert_melt function to test_utils folder

2c33899

update util imports in all files

ec476eb

update util import statements in tests

6daa7dd

split util files into better named files

c7a6f69

split util files into new test files

0328ceb

gwaybio added 2 commits May 12, 2021 14:13

fix assertion tolerance

e7aa8ed

fix np builtin type deprecation in 1.2

c730a0b

revert back to deprecated check_less_precise

03c09ee

gwaybio mentioned this pull request May 12, 2021

Upgrade to minimum pandas 1.2 and deprecate python 3.5 #56

Open

gwaybio requested a review from michaelbornholdt May 12, 2021 18:59

michaelbornholdt suggested changes May 18, 2021

View reviewed changes

add util init file

bdb9c78

gwaybio mentioned this pull request May 18, 2021

Automate black formatting with github action #57

Closed

gwaybio and others added 4 commits May 18, 2021 13:48

Update cytominer_eval/tests/test_operations/test_mp_value.py

ec01949

Co-authored-by: Michael Bornholdt <[email protected]>

Update cytominer_eval/tests/test_utils/test_availability_utils.py

e450cff

Co-authored-by: Michael Bornholdt <[email protected]>

Update cytominer_eval/tests/test_utils/test_grit_utils.py

16df0c7

Co-authored-by: Michael Bornholdt <[email protected]>

Apply suggestions from code review

5d2215c

Fixing spaces after function import, docstring consistency, and a missing numpy builtin update Co-authored-by: Michael Bornholdt <[email protected]>

gwaybio commented May 18, 2021

View reviewed changes

gwaybio requested a review from michaelbornholdt May 18, 2021 17:55

michaelbornholdt approved these changes May 18, 2021

View reviewed changes

gwaybio merged commit 9569721 into cytomining:master May 18, 2021

gwaybio deleted the refactor-utils branch May 18, 2021 18:13

gwaybio mentioned this pull request May 19, 2021

Refactor availability util file and add distribution comparison to grit #58

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor utility functions #55

Refactor utility functions #55

gwaybio commented May 11, 2021 •

edited

Loading

gwaybio commented May 11, 2021

codecov-commenter commented May 12, 2021 •

edited

Loading

gwaybio commented May 12, 2021

michaelbornholdt left a comment •

edited

Loading

michaelbornholdt May 18, 2021

gwaybio May 18, 2021

michaelbornholdt May 18, 2021

michaelbornholdt May 18, 2021

gwaybio May 18, 2021

michaelbornholdt May 18, 2021

gwaybio May 18, 2021

michaelbornholdt May 18, 2021

gwaybio May 18, 2021

michaelbornholdt commented May 18, 2021

gwaybio left a comment

gwaybio May 18, 2021

gwaybio May 18, 2021

gwaybio May 18, 2021

michaelbornholdt left a comment

michaelbornholdt commented May 18, 2021

gwaybio commented May 18, 2021


		from cytominer_eval.utils.availability_utils import get_available_grit_summary_methods

		random.seed(123)

Refactor utility functions #55

Refactor utility functions #55

Conversation

gwaybio commented May 11, 2021 • edited Loading

gwaybio commented May 11, 2021

codecov-commenter commented May 12, 2021 • edited Loading

Codecov Report

gwaybio commented May 12, 2021

michaelbornholdt left a comment • edited Loading

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 May 18, 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

michaelbornholdt left a comment

Choose a reason for hiding this comment

michaelbornholdt commented May 18, 2021

gwaybio commented May 18, 2021

gwaybio commented May 11, 2021 •

edited

Loading

codecov-commenter commented May 12, 2021 •

edited

Loading

michaelbornholdt left a comment •

edited

Loading