docment vision.augment for RandTransform and Item Transforms

fastaidocsprint / fastai

Documentation Sprint for the fastai deep learning library

http://fastaidocsprint.github.io/fastai

Apache License 2.0

15 stars 16 forks source link

docment vision.augment for RandTransform and Item Transforms #57

Closed dienhoa closed 2 years ago

dienhoa commented 2 years ago

This is a simple PR for docment 2 first tasks in vision.augment issue 29 . This is much for me to get used with the workflow and everything is aligned with the style guide

review-notebook-app[bot] commented 2 years ago

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

dienhoa commented 2 years ago

Some notes:

I don't understand clearly what is the use of nm in RandTransform and in discord, Zach said might be it is a bug so I left it blank.
for b in before_call and __call__ I believe it can be anything so not sure we should document it.
split_idx in before_call I forgot to document, I will add it later in the next PR

muellerzr commented 2 years ago

@dienhoa please add it in this PR, so everything is complete :)

dienhoa commented 2 years ago

@dienhoa please add it in this PR, so everything is complete :)

Just add it

marii-moe commented 2 years ago

For nm I am guessing it is dead code.

I checked old version of RandTransform and nothing: https://github.com/fastai/fastai/blame/b1507889980e48dac9a015492bcb1c44da2bd1c2/fastai2/vision/augment.py

Also in fastcore: https://github.com/fastai/fastcore/blame/463e76827090b4607ff8d95ca93cb98af31c2682/fastcore/transform.py

Nothing back to the beginning of time seems to be suggesting nm is there, the history for when it was originally added is missing. I do not think it was being used at the beginning of time either.

dienhoa commented 2 years ago

I've just updated with a new commit. One note: function crop_pad has a resize feature which I think is not obvious. Maybe we need to rename it to something like crop_pad_resize?

warner-benjamin commented 2 years ago

Fix those last suggestions and remove the newlines before the cls objects, per the updated style guide, and pending @marii-moe's comments I believe we can merge this PR.

dienhoa commented 2 years ago

Thanks @warner-benjamin . However, I don't get what you mean by "remove the newlines before the cls objects" and I can not find the updated style guide from the recent commits. Can you pls give an example?

marii-moe commented 2 years ago

@dienhoa self should be on the same line as def before_call for example. As in benjamin's above suggestion.

like this def before_call(self,

not like:

def before_call(
        self,

For me get this correct, and split_idx being train/valid, and before_call being batchwise and I am happy. (as in benjamins suggestions)

dienhoa commented 2 years ago

@warner-benjamin @marii-moe Thank you a lot for your help (and your patience). I've learned a lot not only about the fastai library but also how to document concisely.

I will try my best for the next PR :)