Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Find images #54

Merged
merged 100 commits into from Apr 30, 2020
Merged

Find images #54

merged 100 commits into from Apr 30, 2020

Conversation

jennyfolkesson
Copy link
Contributor

@jennyfolkesson jennyfolkesson commented Apr 27, 2020

Added support to read either a directory containing images or a number of subdirectories containing one image each.

I also added tests to the code, which I recommend doing moving forward for those who are so inclined. It really helps if the repo is intended to have many collaborators.

@mattersoflight
Copy link
Member

mattersoflight commented Apr 29, 2020

@jennyfolkesson I see that the error in well workflow came because of refactoring of the image io methods.

It is great that you started to refactor and are now adding tests in pytest format.

Please implement the following to make sure changes from this branch are useful in master:

  • rebase onto current master and make sure that all tests use pytest library.
  • test array workflows in this branch with Scienion format /Volumes/GoogleDrive/My Drive/ELISAarrayReader/images_scienion/2020-04-08-14-48-49-COVID_concentrationtest_April8_images
  • test well workflows in this branch with /Volumes/GoogleDrive/My Drive/ELISAarrayReader/images_leica/20200408_VanillaELISA_Leica2.5x_2
  • If you like, curate the 5 'best images' and the 5 'worst images' for extract_od workflows (/Volumes/GoogleDrive/My Drive/ELISAarrayReader/data_for_tests_and_github/subset_for_tests).

@codecov-io
Copy link

codecov-io commented Apr 30, 2020

Codecov Report

Merging #54 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #54   +/-   ##
=======================================
  Coverage   22.36%   22.36%           
=======================================
  Files          21       21           
  Lines        1359     1359           
=======================================
  Hits          304      304           
  Misses       1055     1055           
Flag Coverage Δ
#unittests 22.36% <0.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 60cef88...60cef88. Read the comment docs.

@jennyfolkesson
Copy link
Contributor Author

@mattersoflight I've rebased wrt master and checked that it works for array_fit and array_interp.
I checked that it ran for the well_* workflows but not sure what they're trying to do so I can't say they're working, just that they didn't crash. :-)
I also changed existing unittests to pytests.
I didn't pick images, figured that can be saved for another day/branch...
I also added code coverage, so we can see how testing will be getting better over time hopefully.

Please take a look again. Thanks!

Copy link
Member

@mattersoflight mattersoflight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The discovery of image files is little buggy

Comment on lines 52 to 55
for ext in extensions:
search_str = os.path.join(input_dir, '**', ext)
image_names.extend(glob.glob(search_str, recursive=True))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

searching the folder tree recursively returns too many images, and assigns wrong images to the wells.
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above screenshot was taken with following arguments:
-e -i="/Volumes/GoogleDrive/My Drive/ELISAarrayReader/images_scienion/2020-04-08-14-48-49-COVID_concentrationtest_April8_images" -o="~/Documents/images_local/2020-04-08-14-48-49-COVID_concentrationtest_April8_images" -wf=array_fit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tested it on the data paths I didn't get these issues, but I never write outputs in the same directory as the input so that's probably why I missed that. Thanks for not only catching it but also fixing it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tested it on the data paths I didn't get these issues, but I never write outputs in the same directory as the input so that's probably why I missed that. Thanks for not only catching it but also fixing it!

Somewhat related -- do we want to generally allow writing reports and outputs to the same directory as the raw data files? It does cause problems when, for example, searching for excel metadata spreadsheets.

@mattersoflight
Copy link
Member

I've fixed the bug by restricting the filename search to either the input folder or one level down from input folder. The well workflow runs fine now and array_fit workflow runs too, but I don't see an output OD file.

@jennyfolkesson please take a look.

@jennyfolkesson
Copy link
Contributor Author

@mattersoflight I did get an OD file, but I named it similar to the debug excel files. I've now renamed the OD excel file to match the one in the interpolation workflow: 'python_median_ODs.xlsx'. I hope that helps.

@mattersoflight
Copy link
Member

I changed the other two filenames to be consistent too. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants