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

Feature request: Add subtract method to RoiTools #995

Closed
yau-lim opened this issue Jun 27, 2022 · 3 comments · Fixed by #1051
Closed

Feature request: Add subtract method to RoiTools #995

yau-lim opened this issue Jun 27, 2022 · 3 comments · Fixed by #1051
Milestone

Comments

@yau-lim
Copy link

yau-lim commented Jun 27, 2022

Currently, RoiTools supports union and intersection methods directly but not subtraction.

Subtraction has to be done using combineROIs and CombineOp methods, while not difficult, is incoherent as union and intersection is directly available as mentioned above.

I would like to suggest having a RoiTools.subtract(roi1, roi2) method added alongside RoiTools.union(rois) and RoiTools.intersection(rois).

@petebankhead
Copy link
Member

Hi @yau-lim as discussed, I agree this should be added - it would be an easy addition to RoiTools, and requiring CombineOp is awkward (not least because it's not obvious that it exists...).

I suppose the difference with union and intersection is that they make sense with any number of ROIs (and so take a collection/list as input), but subtraction only really makes sense with two.... or at least that could have been what I was thinking originally.

With that in mind, one option is to add

public static ROI subtract(ROI roi1, ROI roi2) {
    return combineROIs(roi1, roi2, CombineOp.SUBTRACT);
}

as a convenience method. But I wonder if it might be even more useful to add

public static ROI subtract(ROI baseROI, ROI... roisToSubtract) {
  // Loop through one or more roisToSubtract, and remove then from baseROI
}

What do you think? Would this output match your expectation for 'subtraction that supports multiple arguments'?

The 'optional array' syntax proposed above would support all the following:

def roiOutput1 = RoiTools.subtract(roi1) // Returns roi1 unchanged
def roiOutput2 = RoiTools.subtract(roi1, roi2) // Returns roi1 with pixels in roi2 removed
def roiOutput3 = RoiTools.subtract(roi1, roi2, roi3, roi4) // Returns roi1 with pixels in the union of roi2, roi3 and roi4 removed

Alternatively, we could stick with just two arguments and then require

def roiOutput3 = RoiTools.subtract(roi1, RoiTools.union(roi2, roi3, roi4))

to get the third output above (albeit perhaps less efficiently).

@petebankhead petebankhead added this to the v0.4.0 milestone Jun 27, 2022
@yau-lim
Copy link
Author

yau-lim commented Jun 27, 2022

def roiOutput3 = RoiTools.subtract(roi1, RoiTools.union(roi2, roi3, roi4))

makes sense to me by building/layering functions together logically but from your other suggestions, maybe

public static ROI subtract(ROI baseROI, Collection<ROI> roisToSubtract) {
  // Loop through one or more roisToSubtract, and remove them from baseROI
}

will make it clear/less ambiguous to which ROIs are being subtracted from the baseROI, e.g.

def roiOutput = RoiTools.subtract(baseROI, [roi1, roi2, roi3, ...])

@MichaelSNelson
Copy link

Either way sounds good, or both ways - there have been a couple times when I expected this function to exist, was confused, and went and looked up that it did not :) And then forgot again, of course.

@petebankhead petebankhead removed their assignment Sep 6, 2022
petebankhead added a commit to petebankhead/qupath that referenced this issue Sep 8, 2022
Fixes qupath#995 and adds a bit more.
Also support varargs with `RoiTools.intersection` and `RoiTools.union`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants