Skip to content

Conversation

tj-schultz
Copy link
Contributor

@tj-schultz tj-schultz commented Feb 19, 2024

Describe your changes
Updated segment_sort to utilize params.sample_label when accessing output observations for trimming the tips to remove the stem. The KeyError was caused by the call to find_tips within segment_sort with no label parameter. find_tips therefore uses the params.sample_label to record the observations and thus if it is not "default", segment_sort fails.

segment_sort seems to be the only morphology function to access outputs.observations in this way; segment_curvature uses a literal key 'backend' to store segment_euclidean_length and segment_path_length observations before calculating curvature, but accesses find_tips output by return image and applying _cv2_findcontours, similarly to other functions.

Type of update

  • Bug fix

Associated issues
#1427

Additional context
Add any other context about the problem here.

For the reviewer
See this page for instructions on how to review the pull request.

  • PR functionality reviewed in a Jupyter Notebook
  • All tests pass
  • Test coverage remains 100%
  • Documentation tested
  • New documentation pages added to plantcv/mkdocs.yml
  • Changes to function input/output signatures added to updating.md
  • Code reviewed
  • PR approved

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fbafe09) 99.98% compared to head (4bb9425) 99.98%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1458   +/-   ##
=======================================
  Coverage   99.98%   99.98%           
=======================================
  Files         158      158           
  Lines        6983     6984    +1     
=======================================
+ Hits         6982     6983    +1     
  Misses          1        1           
Flag Coverage Δ
unittests 99.98% <100.00%> (ø)

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

Files Coverage Δ
plantcv/plantcv/morphology/segment_sort.py 100.00% <100.00%> (ø)

@codecov-commenter
Copy link

codecov-commenter commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.98%. Comparing base (a5ede9b) to head (4bb9425).
Report is 51 commits behind head on main.

❗ Current head 4bb9425 differs from pull request most recent head 93db8d8. Consider uploading reports for the commit 93db8d8 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1458      +/-   ##
==========================================
- Coverage   99.98%   99.98%   -0.01%     
==========================================
  Files         161      158       -3     
  Lines        7066     6984      -82     
==========================================
- Hits         7065     6983      -82     
  Misses          1        1              
Flag Coverage Δ
unittests 99.98% <100.00%> (-0.01%) ⬇️

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

Files Coverage Δ
plantcv/plantcv/morphology/segment_sort.py 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

Copy link

deepsource-io bot commented Mar 2, 2024

Here's the code health analysis summary for commits 51a9339..93db8d8. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource Python LogoPython✅ SuccessView Check ↗
DeepSource Test coverage LogoTest coverage⚠️ Artifact not reportedTimed out: Artifact was never reportedView Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

@nfahlgren nfahlgren merged commit 40d2797 into danforthcenter:main Mar 2, 2024
@nfahlgren
Copy link
Member

@all-contributors please add @tj-schultz for code, bug

Copy link
Contributor

@nfahlgren

I've put up a pull request to add @tj-schultz! 🎉

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

Successfully merging this pull request may close these issues.

4 participants