-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
6699 random sort grid patch #6701
Conversation
Signed-off-by: jmnolte <[email protected]>
…te/MONAI into 6699-random-sort-GridPatch "Add random sorting option to sort_fn argument in GridPatch transform"
I, jmnolte <[email protected]>, hereby add my Signed-off-by to this commit: d1829f3 Signed-off-by: jmnolte <[email protected]>
Signed-off-by: jmnolte <[email protected]>
Signed-off-by: jmnolte <[email protected]>
for more information, see https://pre-commit.ci
Signed-off-by: jmnolte <[email protected]>
Signed-off-by: jmnolte <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmnolte Thanks for your contribution. The only downside of implementing the randomness is GridPatch
is that this transform is supposed to be non-random while relying on its random counter part RandGridPatch
to include randomness. Thus, I would suggest to only modify RandGridPatch
for this purpose by overriding its filter_count
method, handling situation with sort_fn="random"
, and delegating the rest to the its parent class.
def filter_count(self, image_np: NdarrayOrTensor, locations: np.ndarray) -> tuple[NdarrayOrTensor, np.ndarray]:
if self.sort_fn == GridPatchSort.RANDOM:
pass # do your logic for random sorting here
return image_np, locations
return super().filter_count(image_np, locations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it would be great if you can update docstring for RandGridPatchd
(the dictionary version of the transform) for this new option ("random"
) too. Thanks
Signed-off-by: jmnolte <[email protected]>
for more information, see https://pre-commit.ci
Thanks for the feedback! I have now created a new |
Co-authored-by: YunLiu <[email protected]> Signed-off-by: Jakob Nolte <[email protected]>
Co-authored-by: YunLiu <[email protected]> Signed-off-by: Jakob Nolte <[email protected]>
Signed-off-by: jmnolte <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, it looks good to me -- please help fix the typing error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick update, LGTM. Could you please help also update the docstring in the RandGridPatchd
?
…Patch Signed-off-by: jmnolte <[email protected]>
I am not quite sure, what's causing the typing error. It appears that the index is of an invalid type but I don't see how the index type returned from |
sure, I'll have a look and trigger more tests @jmnolte. |
…dPatch Signed-off-by: Wenqi Li <[email protected]>
Signed-off-by: Wenqi Li <[email protected]>
/build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the comments. LGTM!
Description
The PR introduces a random sorting option to the
GridPatch, RandGridPatch, GridPatchd, RandGridPatchd
transforms. So far, the transforms allowed patches to be sorted according to the sum of their intensity or in their default order (i.e., sliding-window manner). With this PR, random extraction of patches as described in Myronenko et al. is enabled.Types of changes