Skip to content

draw_roi bugfix #1511

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 8, 2024
Merged

draw_roi bugfix #1511

merged 5 commits into from
May 8, 2024

Conversation

HaleySchuhl
Copy link
Contributor

draw the ROI bounds regardless of whether the moments can be calculated, and only plot number on debug image when there is more than one ROI

Describe your changes
Un-indent the piece of code that is plotting the ROI boundaries, not requirement to be able to calculate moments to be able to draw this. Also adds a check for number of ROIs getting drawn and only adds number when >1.

Type of update
Is this a:

  • Bug fix and/or feature enhancement

Associated issues

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

@HaleySchuhl
draw_roi bugfix
0cbb5c9 
draw the ROI bounds regardless of whether the moments can be calculated,and only plot number when more than one ROI
@HaleySchuhl HaleySchuhl added ready to review update Updates an existing feature/method cleanup Code quality updates labels May 2, 2024
@HaleySchuhl HaleySchuhl added this to the PlantCV v4.3 milestone May 2, 2024
@HaleySchuhl HaleySchuhl self-assigned this May 2, 2024
@deepsource-io deepsource.io
Copy link

deepsource-io bot commented May 2, 2024 •
edited
Loading

Here's the code health analysis summary for commits 4f13522..b7b397f. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource Python LogoPython✅ SuccessView Check ↗
DeepSource Test coverage LogoTest coverage✅ SuccessView Check ↗

Code Coverage Report

MetricAggregatePython
Branch Coverage100%100%
Composite Coverage99.7%99.7%
Line Coverage99.7%99.7%
New Branch Coverage100%100%
New Composite Coverage100%100%
New Line Coverage100%, ✅ Above Threshold100%, ✅ Above Threshold
💡 If you’re a repository administrator, you can configure the quality gates from the settings.

@nfahlgren
Copy link
Member

In the previous PR I intentionally changed it to plot the ROI number even if there is only 1 ROI

@HaleySchuhl
Copy link
Contributor Author

In the previous PR I intentionally changed it to plot the ROI number even if there is only 1 ROI

Ah ok I see @nfahlgren , I found it a little confusing to start plotting a 0 when there is just one, but maybe that's just my personal preference. While using the updated functionality I thought there was a centroid getting drawn at first but depending on resolution & params.text_size it's more or less obvious what the oval annotation is.
image

image

@nfahlgren
Copy link
Member

In outputs we number the output labels from 1 so I think we should also number the ROIs from 1 instead of 0 now that I look at it

@HaleySchuhl
Copy link
Contributor Author

Yea true, that'll be less confusing all around. Easy enough to implement, I can do that now.

@HaleySchuhl HaleySchuhl requested a review from nfahlgren May 2, 2024 21:13
@nfahlgren nfahlgren merged commit 6c9d034 into main May 8, 2024
@nfahlgren nfahlgren deleted the minor_roi_debug_update branch May 8, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfahlgren nfahlgren nfahlgren approved these changes

Assignees

@HaleySchuhl HaleySchuhl

Labels
cleanup Code quality updates ready to review update Updates an existing feature/method
Projects
PlantCV4
Status: Done
Milestone
PlantCV v4.3
Development

Successfully merging this pull request may close these issues.
2 participants
@HaleySchuhl @nfahlgren