+
Skip to content

Add RAG threshold parameters to allow return labels and masking #300

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 8 commits into from
Nov 15, 2021
Merged

Add RAG threshold parameters to allow return labels and masking #300

merged 8 commits into from
Nov 15, 2021

Conversation

kheffah
Copy link
Collaborator

@kheffah kheffah commented Jul 23, 2021 •
edited
Loading

Description

Flexible rag filter allowing:

  1. Masked filtering to exclude, for example, areas outside the tissue
  2. Returning labeled mask for individual clustered segments instead of a colored RGB. That way, the returned clusters can be used in downstream analysis.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

None

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@codecov
Copy link

codecov bot commented Jul 23, 2021 •
edited
Loading

Codecov Report

Merging #300 (063b104) into master (29ebb9a) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #300   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines         1328      1331    +3     
  Branches       133       134    +1     
=========================================
+ Hits          1328      1331    +3
Impacted Files Coverage Δ
histolab/filters/image_filters.py 100.00% <100.00%> (ø)
histolab/filters/image_filters_functional.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29ebb9a...063b104. Read the comment docs.

alessiamarcolini
alessiamarcolini requested changes Jul 26, 2021
View reviewed changes
Copy link
Collaborator

@alessiamarcolini alessiamarcolini left a comment

Choose a reason for hiding this comment

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

Some comments about docstring style and testing best practices

@alessiamarcolini alessiamarcolini added this to the 0.2.8 milestone Jul 26, 2021
@alessiamarcolini alessiamarcolini added the enhancement New feature or request label Jul 26, 2021
@kheffah
Copy link
Collaborator Author

kheffah commented Jul 26, 2021

@alessiamarcolini OK, all done 👍🏻

@alessiamarcolini
Copy link
Collaborator

@kheffah could you please squash all the "update" commit into a single one? Thanks!!

PS there is a way to add all the suggestions to a single commit from the interface directly

@kheffah
Copy link
Collaborator Author

kheffah commented Jul 26, 2021

@alessiamarcolini Oops I didn't know that. OK, now commit aaf4a09 contains the squashed changes.

@alessiamarcolini
Copy link
Collaborator

Hi @kheffah, sorry I meant to do an interactive rebase to rewrite history and have only the relevant commits (with a proper message) to have a clean history. Please let me know if you need any help with this :)

@kheffah
Copy link
Collaborator Author

kheffah commented Jul 29, 2021 •
edited
Loading

Hi @kheffah, sorry I meant to do an interactive rebase to rewrite history and have only the relevant commits (with a proper message) to have a clean history. Please let me know if you need any help with this :)

Yes please, I've never done that before. I'm dealing with a deadline at the moment so maybe at least a few days till I get to it. If you prefer to get it done quickly, feel free to do it :)

@kheffah
Copy link
Collaborator Author

kheffah commented Jul 31, 2021 •
edited
Loading

@alessiamarcolini OK I officially give up 😆 Everytime I do the interactive rebase locally I get a conflict, asks me to resolve it, asks me to pull, and then when the rebase succeeds and I push, I still see the commits. I could probably fix this if I spend more time on it but I have high priority research tasks at the time being. But feel free to do it if you'd like, sorry about that.

@alessiamarcolini alessiamarcolini force-pushed the flexible-rag branch 2 times, most recently from a7e69b5 to 3607636 Compare August 2, 2021 15:14
ernestoarbitrio
ernestoarbitrio requested changes Aug 2, 2021
View reviewed changes
Copy link
Member

@ernestoarbitrio ernestoarbitrio left a comment

Choose a reason for hiding this comment

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

per comments

alessiamarcolini
alessiamarcolini requested changes Aug 2, 2021
View reviewed changes
Copy link
Collaborator

@alessiamarcolini alessiamarcolini left a comment

Choose a reason for hiding this comment

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

Last stylistic changes :)

PS after my rebase (which rewrote history) is better for you if you work on a different clone of the repo (only required for upcoming work on this branch)

@ernestoarbitrio
Copy link
Member

@kheffah I think after these last comments from me and @alessiamarcolini this PR should be ready to go :D Thanks for your work

alessiamarcolini
alessiamarcolini reviewed Aug 2, 2021
View reviewed changes
Copy link
Collaborator

@alessiamarcolini alessiamarcolini left a comment

Choose a reason for hiding this comment

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

I also asked a review to @nicolebussola if she has time ;)

@ernestoarbitrio
Copy link
Member

I also asked a review to @nicolebussola if she has time ;)

Oh ... got it, but I think 2 reviews are more than enough 😄

ernestoarbitrio
ernestoarbitrio reviewed Aug 2, 2021
View reviewed changes
@kheffah
Copy link
Collaborator Author

kheffah commented Aug 2, 2021

@alessiamarcolini @ernestoarbitrio Thanks! All suggestions incorporated now.

alessiamarcolini
alessiamarcolini approved these changes Aug 10, 2021
View reviewed changes
Copy link
Collaborator

@alessiamarcolini alessiamarcolini left a comment

Choose a reason for hiding this comment

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

LGTM

nicolebussola
nicolebussola approved these changes Aug 10, 2021
View reviewed changes
Copy link
Collaborator

@nicolebussola nicolebussola left a comment

Choose a reason for hiding this comment

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

Nice improvement

@alessiamarcolini alessiamarcolini modified the milestones: 0.2.8, 0.4.0 Nov 8, 2021
@alessiamarcolini alessiamarcolini merged commit 97abd62 into master Nov 15, 2021
@alessiamarcolini alessiamarcolini deleted the flexible-rag branch November 15, 2021 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ernestoarbitrio ernestoarbitrio ernestoarbitrio approved these changes

@alessiamarcolini alessiamarcolini alessiamarcolini approved these changes

@nicolebussola nicolebussola nicolebussola approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request ready-for-merge
Projects
None yet
Milestone
0.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@kheffah @alessiamarcolini @ernestoarbitrio @nicolebussola
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载