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

Add .correct_mask method #27

Open
wants to merge 166 commits into
base: main
Choose a base branch
from
Open

Add .correct_mask method #27

wants to merge 166 commits into from

Conversation

HaleySchuhl
Copy link
Contributor

@HaleySchuhl HaleySchuhl commented May 14, 2024

Describe your changes
Rewrite the clickcount_correct method, used to add and remove to make a corrected_mask, which makes a labeled mask containing all objects that are resolvable in the input binary mask, based on Point annotations. Outputs can be used for size analysis. Unresolved objects cannot be measured but can still keep info about count.

No longer includes the _recover_circ part of the algorithm which was changing the shape of objects in some cases and was too specific to the size & shape of pollen.

Type of update
Is this a:

  • New feature

Associated issues
Reference associated issue numbers. Does this pull request close any issues?

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

@HaleySchuhl HaleySchuhl added work in progress Mark work in progress new feature New feature ideas and solutions labels May 14, 2024
@HaleySchuhl HaleySchuhl self-assigned this May 14, 2024
Copy link

deepsource-io bot commented May 14, 2024

Here's the code health analysis summary for commits 447f2ab..20f968d. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource Python LogoPython❌ Failure
❗ 3 occurences introduced
View Check ↗
DeepSource Test coverage LogoTest coverage✅ SuccessView Check ↗

Code Coverage Report

MetricAggregatePython
Branch Coverage100%100%
Composite Coverage100%100%
Line Coverage100%100%
New Branch Coverage100%100%
New Composite Coverage100%100%
New Line Coverage100%100%

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

@HaleySchuhl HaleySchuhl added the ready to review Ready for code review label Aug 20, 2024
@HaleySchuhl HaleySchuhl mentioned this pull request Sep 3, 2024
11 tasks
Copy link

@annacasto annacasto left a comment

Choose a reason for hiding this comment

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

Everything seems to work as intended. I put the notebook I used for testing in the Google Drive folder. It seems like there's just a test coverage issue and one deep source issue to fix. Once the coverage is good to go, I can re-run the tests and approve though!

@HaleySchuhl
Copy link
Contributor Author

I'm not certain we will be able to decrease the complexity, which is the deepsource complaint. But coverage is fixed now @annacasto at least!

Copy link

@annacasto annacasto left a comment

Choose a reason for hiding this comment

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

There's one deep source issue about code complexity that we're not sure can be resolved, so approving for now since tests pass and it was reviewed for functionality.

Copy link
Member

@nfahlgren nfahlgren left a comment

Choose a reason for hiding this comment

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

I added some suggestions to reduce complexity

plantcv/annotate/classes.py Outdated Show resolved Hide resolved
plantcv/annotate/classes.py Outdated Show resolved Hide resolved
plantcv/annotate/classes.py Outdated Show resolved Hide resolved
plantcv/annotate/classes.py Show resolved Hide resolved
Comment on lines 181 to 186
for names in labelnames:
for (x, y) in self.coords[names]:
x = int(x)
y = int(y)
# Draw pt annotations onto a blank mask
pts_mask = cv2.circle(pts_mask, (x, y), radius=0, color=(255), thickness=-1)
Copy link
Member

Choose a reason for hiding this comment

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

Take a look at itertools, particularly itertools.product I think to reduce nested for loops to one loop. This also applies to dealing with complexity below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I have simplified this helper function.

I spent most of this morning testing different ways to loop through all annotation coordinates, but I'm struggling to find a way to avoid the nested for loop within the actual correct_mask method. The part that I'm not sure how to get around is that the algorithm currently relies on being aware of the corresponding class label called names, but I believe it's possible. If you have more suggestions I'm happy to implement them, and I'm also amenable if you want to make changes directly.

Copy link
Member

Choose a reason for hiding this comment

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

itertools.product?

Copy link
Contributor Author

@HaleySchuhl HaleySchuhl Sep 18, 2024

Choose a reason for hiding this comment

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

I have removed the nested for loop with a different approach (edited) butitertools.product is the first way I tried this, and I'm just not sure how to use it in such a way that keeps awareness of the key part of the dictionary rather than just gathering all coordinates. Here's the closest I got today:

import itertools

# still a bad product
for instance in itertools.product(counter.coords.keys(), counter.coords.values()):
    print(instance)
('default', [(398, 416), (1262, 338), (1701, 378), (1806, 1025), (1518, 1505), (556, 1430), (481, 997), (1090, 754)])
('default', [(321, 486), (308, 888), (333, 993), (321, 1193), (338, 1489)])
('default', [(1423, 529), (1721, 399), (1864, 229)])
('germinated', [(398, 416), (1262, 338), (1701, 378), (1806, 1025), (1518, 1505), (556, 1430), (481, 997), (1090, 754)])
('germinated', [(321, 486), (308, 888), (333, 993), (321, 1193), (338, 1489)])
('germinated', [(1423, 529), (1721, 399), (1864, 229)])
('other', [(398, 416), (1262, 338), (1701, 378), (1806, 1025), (1518, 1505), (556, 1430), (481, 997), (1090, 754)])
('other', [(321, 486), (308, 888), (333, 993), (321, 1193), (338, 1489)])
('other', [(1423, 529), (1721, 399), (1864, 229)])

I found that this sum syntax I've added to _create_pts_mask does a nice job of combining all annotations into a single list, but for the algorithm to utilize this format of data, we would also need to create a list of the same length that contains the class label corresponding to each value. (I'm currently doing this with a for loop over all annotations and looking up the key for each value)

I'm just not certain that itertools.product is the method that can create this based on the googling & testing I was doing today, but that could be my lack of familiarity with the package. So if somebody else wants to replace the logic then feel welcomed to directly commit those improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature ideas and solutions ready to review Ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants