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

Fix ImportError in plantcv caused by change in skimage.feature function name #998

Merged
merged 6 commits into from
Mar 14, 2023

Conversation

fbarbe00
Copy link
Contributor

@fbarbe00 fbarbe00 commented Mar 3, 2023

Currently, when installing plantcv, the following error is raised: ImportError: cannot import name 'greycomatrix' from 'skimage.feature'
This is because skimage.feature changed their function name from greycomatrix to graycomatrix (see https://stackoverflow.com/questions/67537650/cannot-import-name-graycomatrix-from-skimage-feature ) from version 0.19. This commit changes the requirements.txt file and the function name.

…on name

This commit updates the requirements.txt file and replaces the use of
the deprecated function name 'greycomatrix' with 'graycomatrix' in
`threshold_methods.py`. The change in function name in skimage.feature
from version 0.19 caused an ImportError when importing plantcv, which is
resolved with this commit.
@HaleySchuhl
Copy link
Contributor

@fbarbe00 thanks very much for opening this PR and sorry for the delay in response.

@HaleySchuhl
Copy link
Contributor

HaleySchuhl commented Mar 13, 2023

Ugh..Looks like the renaming issues also trickle down past the imports part.

Now tests fail on

  • plantcv/plantcv/closing.py:26: in closing --> E TypeError: binary_closing() got an unexpected keyword argument 'selem'
  • The watershed_segmentation function --> E TypeError: peak_local_max() got an unexpected keyword argument 'indices'

If you'd prefer, I can dig into their updated parameter names, push commits to this PR, and hopefully resolve those remaining bugs. Thanks again @fbarbe00 for bringing this to our attention. @nfahlgren can we add them as a contributor so tests automatically run next time?

Update:
I pushed two commits to address the updates to parameter names from the two functions I mentioned. However, the watershedd_segmentation function still isn't un-broken since the output of skimage.feature.peak_local_max is now returns The coordinates of the peaks and doesn't seem to give us the option any longer to change indices (If False, the output will be a boolean array shaped as image.shape with peaks present at True elements).

https://scikit-image.org/docs/0.20.x/api/skimage.feature.html#peak-local-max

vs.

https://scikit-image.org/docs/0.18.x/api/skimage.feature.html#peak-local-max

@fbarbe00
Copy link
Contributor Author

Thanks a lot for your reply! Is there anything I can still try to help with?

@fbarbe00
Copy link
Contributor Author

Btw, this solves Issue #1002

@HaleySchuhl
Copy link
Contributor

Yes @fbarbe00 If you're able to figure out how to get an array-like output from the latest version of skimage.feature.peak_local_maxskimage.feature.peak_local_max . TThe that functionality isn't recoverable I might suggest pinning the version down to the older functionality.

In skimage>=0.19, the `peak_local_max` function no longer takes the
`indices` argument. This adds two lines to compute the indices after
getting the local max coordinates.
@fbarbe00
Copy link
Contributor Author

Hi @HaleySchuhl , I believe my last commit fixes it! I just looked at what the peak_local_max function did in v0.18 (https://github.com/scikit-image/scikit-image/blob/v0.18.x/skimage/feature/peak.py) , and looks like indices=False only affected two lines at the end. These lines have now been added to plantcv.

Note that I do not have a deep understanding of what this code actually does, but this does seem to fix this function and work as before.

@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #998 (2b16a1d) into main (df07ef6) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 2b16a1d differs from pull request most recent head 61f4fca. Consider uploading reports for the commit 61f4fca to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##              main     #998      +/-   ##
===========================================
- Coverage   100.00%   99.98%   -0.02%     
===========================================
  Files          129      141      +12     
  Lines         5803     6004     +201     
===========================================
+ Hits          5803     6003     +200     
- Misses           0        1       +1     
Flag Coverage Δ
unittests 99.98% <100.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
plantcv/plantcv/closing.py 100.00% <100.00%> (ø)
plantcv/plantcv/threshold/threshold_methods.py 100.00% <100.00%> (ø)
plantcv/plantcv/watershed.py 100.00% <100.00%> (ø)

... and 14 files with indirect coverage changes

@HaleySchuhl
Copy link
Contributor

Awesome! Tests seem to pass but now there's an issue in the Python 3.7 build on codecov for some reason? ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')

@nfahlgren nfahlgren added the bugfix Bug fixes label Mar 14, 2023
@nfahlgren nfahlgren added this to In progress in PlantCV3 via automation Mar 14, 2023
@nfahlgren
Copy link
Member

I think codecov stopped working with Python 3.7, which was the oldest version we were testing. I bumped up the testing environments to 3.8, 3.9, and 3.10 since 3.11 is out now.

@nfahlgren nfahlgren merged commit 998a8a6 into danforthcenter:main Mar 14, 2023
PlantCV3 automation moved this from In progress to Done Mar 14, 2023
@HaleySchuhl HaleySchuhl mentioned this pull request Mar 14, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bug fixes
Projects
No open projects
PlantCV3
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants