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

Docment vision.augment Affine Tfms to the end #92

Merged
merged 15 commits into from
Mar 13, 2022
Merged

Docment vision.augment Affine Tfms to the end #92

merged 15 commits into from
Mar 13, 2022

Conversation

dienhoa
Copy link

@dienhoa dienhoa commented Mar 4, 2022

Docment for this issue: #29 . From Affine Tfms to the end

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@dienhoa dienhoa changed the title Docment vision.augment Lighting Tfms to the end Docment vision.augment Affine Tfms to the end Mar 4, 2022
@dienhoa
Copy link
Author

dienhoa commented Mar 4, 2022

I accidentally make this PR based on this PR #89 which hasn't been finished yet. Then it creates a big big PR. Hope we can step by step merging it

@@ -362,13 +362,25 @@ def _grid_sample(x, coords, mode='bilinear', padding_mode='reflection', align_co
return F.grid_sample(x, coords, mode=mode, padding_mode=padding_mode, align_corners=align_corners)

# Cell
def affine_grid(theta, size, align_corners=None):
def affine_grid(
theta:Tensor, # Transformation `Tensor`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
theta:Tensor, # Transformation `Tensor`
theta:Tensor, # A batch of Affine transformation matrices

This is the same as mat in affine_coord

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. I fix it in the new commit

def __init__(self, aff_fs=None, coord_fs=None, size=None, mode='bilinear', pad_mode=PadMode.Reflection,
mode_mask='nearest', align_corners=None, **kwargs):
def __init__(self,
aff_fs:(callable,[callable])=None, # Affine transformations function for a batch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
aff_fs:(callable,[callable])=None, # Affine transformations function for a batch
aff_fs:(callable,list)=None, # Affine transformations function for a batch

Just list for ones like these instead of [callable]

Copy link
Author

Choose a reason for hiding this comment

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

Done. I still have a type like this in this PR. example draw:(int, [int], callable) Do you think I need to change [int] to list? I believe the user can be confused to use a list of callable or list of int.

Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dienhoa yes, because [int] will cause errors for users that use IDEs.

Copy link
Author

Choose a reason for hiding this comment

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

I change very [type] to list in the new commit

Copy link
Collaborator

@warner-benjamin warner-benjamin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I left a couple comments and suggestions which are applicable to many of the doc strings. Please review and apply to the rest of the doc strings as needed.

fastai/vision/augment.py Outdated Show resolved Hide resolved
fastai/vision/augment.py Outdated Show resolved Hide resolved
fastai/vision/augment.py Outdated Show resolved Hide resolved
fastai/vision/augment.py Outdated Show resolved Hide resolved
@dienhoa
Copy link
Author

dienhoa commented Mar 7, 2022

Thanks @warner-benjamin . Update a new commit with your suggestions.

@marii-moe
Copy link
Collaborator

marii-moe commented Mar 9, 2022

@dienhoa Did you do anything that might cause no_random to make this notebook non-deterministic for the no-random wrapped pieces? When I run the code I get the same results as the notebook so just wondering. I think that could be a bug in no_random.

@dienhoa
Copy link
Author

dienhoa commented Mar 9, 2022

@dienhoa Did you do anything that might cause no_random to make this notebook non-deterministic for the no-random wrapped pieces? When I run the code I get the same results as the notebook so just wondering. I think that could be a bug in no_random.

I'm not sure but do we need to, after experimenting, restart and run all the notebooks? I usually play around with the notebook and once finish, I run nbdev_build_lib -> 'nbdev_build_doc' -> 'nbdev_clean_nbs'. Is play a cell multiple times create a different version of output?

@marii-moe
Copy link
Collaborator

@dienhoa no_random should always return the same result, even if you run the cell multiple times, which is why I'm curious.

Copy link
Collaborator

@warner-benjamin warner-benjamin left a comment

Choose a reason for hiding this comment

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

In general, we shouldn't be checking in example changes if the code hasn't changed. If you could revert those, the rest looks good to me.

Copy link
Collaborator

@marii-moe marii-moe left a comment

Choose a reason for hiding this comment

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

Okay just my list [callable] and see if you can get the old images back, and we can commit this one.

max_warp:float=0.2, # Maximum value of changing warp per
p_affine:float=0.75, # Probability of applying affine transformation
p_lighting:float=0.75, # Probability of changing brightnest and contrast
xtra_tfms:[callable]=None, # Custom Transformations
Copy link
Collaborator

@marii-moe marii-moe Mar 9, 2022

Choose a reason for hiding this comment

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

Suggested change
xtra_tfms:[callable]=None, # Custom Transformations
xtra_tfms:list=None, # Custom Transformations

Make this change, and I think we are mostly good from my perspective.

Copy link
Collaborator

Choose a reason for hiding this comment

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

"List of" strikes me as unnecessary since the type hint is list

Copy link
Collaborator

@marii-moe marii-moe Mar 11, 2022

Choose a reason for hiding this comment

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

Hmm, I agree just "Custom Transformations" as it was originally is good. The type just needs to be "list. I'll edit the suggestion.

@dienhoa
Copy link
Author

dienhoa commented Mar 11, 2022

I can not make no_random reproduce the same results as in master. I revert the output in my last commit to the output of master by VSCode.

There are some images that, visually I found no differences but notebook_review_app still doesn't like it.

Copy link
Collaborator

@marii-moe marii-moe left a comment

Choose a reason for hiding this comment

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

LGTM

@marii-moe marii-moe merged commit 4a1fc62 into fastaidocsprint:master Mar 13, 2022
@dienhoa dienhoa deleted the vision.augment4 branch March 13, 2022 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants