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

Randomization of transforms per image per batch #231

Merged
merged 3 commits into from
Mar 31, 2022

Conversation

shekhardw
Copy link
Contributor

As per #225

Randomness updated to be applied to per image in the batch. Existing functionality of applying randomness on entire batch at once is unchanged.

@shekhardw shekhardw added the improvement Improves an existing functionality label Mar 2, 2022
@shekhardw shekhardw requested a review from a team as a code owner March 2, 2022 00:08
@shekhardw shekhardw self-assigned this Mar 2, 2022
@shekhardw shekhardw added the non-breaking Introduces a non-breaking change label Mar 2, 2022
@chirayuG-nvidia
Copy link
Contributor

Changes look fine to me for rand_color_jitter. Performance can be improved by doing some sort of kernel aggregation and selective image level filtering within the kernel. Once we have proved that rand_color_jitter is good for model accuracy/froc scores, can look into optimizing performance for this.

Copy link
Contributor

@gigony gigony 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 to me.
Have some minor suggestions.

python/cucim/src/cucim/core/operations/color/jitter.py Outdated Show resolved Hide resolved
np_output = spt.rand_image_flip(arr_batch, prob=1.0, spatial_axis=(2, 3))
np_output = spt.rand_image_flip(arr_batch,
prob=1.0,
spatial_axis=(2, 3),
Copy link
Contributor

Choose a reason for hiding this comment

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

Parameter input is not consistent. Here it uses (2, 3), but below code, it uses [2, 3].

@@ -130,7 +131,8 @@ def image_rotate_90(
def rand_image_flip(
img: Any,
spatial_axis: tuple(),
prob: float = 0.1
prob: float = 0.1,
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the good default value for this?
Having 0.5 for the default value looks more useful here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update it.

@jakirkham
Copy link
Member

jakirkham commented Mar 30, 2022

Generally seems reasonable. Do we want to test whole_batch=False as well?

Edit: Filed issue ( #259 ) to track this.

@jakirkham
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 7602774 into rapidsai:branch-22.04 Mar 31, 2022
@gigony gigony changed the title randomization per image per batch Randomization of transforms per image per batch Apr 7, 2022
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants