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

remove max_val param from threshold.binary() #1210

Merged

Conversation

afinit
Copy link
Collaborator

@afinit afinit commented Jun 28, 2023

Describe your changes

Removes max_val param from the threshold.binary method

Type of update

feature enhancement

Associated issues

Accomplishes 1 task in this ticket: #1194

Additional context

There are a few tests failing for me in the release-4.0. After these changes, no new tests are failing.

@HaleySchuhl @nfahlgren This morning I sent you both an email about potentially contributing to this project. Figured I'd just push a PR while I waited for a response. Please feel free to mark this up as much as you want to make sure I'm properly contributing.

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

@@ -48,7 +46,7 @@ def binary(gray_img, threshold, max_value, object_type="light"):
params.device += 1

# Threshold the image
bin_img = _call_threshold(gray_img, threshold, max_value, threshold_method, "_binary_threshold_")
bin_img = _call_threshold(gray_img, threshold, 255, threshold_method, "_binary_threshold_")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

happy to update this to keep max_value as a variable here and set that to 255 above in the function if that's preferred

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm @nfahlgren do we want to remove the argument from the helper function too? I don't imagine we'd ever set it to anything else...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think removing it is fine

@afinit
Copy link
Collaborator Author

afinit commented Jun 28, 2023

It looks like there's a build issue. I'm wondering if that's just a "rerun and it should be fine" situation, or somehow related to the fact that I made this pull request from a fork?

@nfahlgren
Copy link
Member

Hi Mark, you were right, the CI pipeline failed to upload coverage results to CodeCov and that's why it showed as failing. I reran the analysis and it passed this time.

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #1210 (03c0da5) into release-4.0 (406e214) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##           release-4.0     #1210   +/-   ##
=============================================
  Coverage       100.00%   100.00%           
=============================================
  Files              163       163           
  Lines             6994      6994           
=============================================
  Hits              6994      6994           
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

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

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

@afinit afinit added this to the PlantCV v4.0 milestone Jun 29, 2023
Copy link
Contributor

@HaleySchuhl HaleySchuhl left a comment

Choose a reason for hiding this comment

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

Looks good! We might want to remove the max_value parameter from the helper function _call_threshold in a future PR

@nfahlgren nfahlgren merged commit cb720cf into danforthcenter:release-4.0 Jun 30, 2023
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.

3 participants