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

Mixup docment #66

Closed kevinbird15 closed 2 years ago

kevinbird15 commented 2 years ago

I think my local branch is slightly ahead of the docment branch because of recent fastai changes Because of that, I'm not going to worry too much about the files that changed besides the three mixup unless somebody wants to help me fix it so it only changes the files I actually am meaning to:

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

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

warner-benjamin commented 2 years ago

This references issue: #48

A couple of comments:

from __future__ import annotations shouldn't be added (yet).

I think this would be better formatted as:

class MixUp(MixHandler):
    "Implementation of https://arxiv.org/abs/1710.09412"
    def __init__(self,
        alpha=.4 # Determine `Beta` Distribution (0.,inf]
    ): 
        super().__init__(alpha)

and the same for similarly formatted methods.

And for rand_bbox I think if there are newlines there should be docments for those args.

hamelsmu commented 2 years ago

~Merging this could cause conflicts for those PRs you merge after this one - the diff on this PR is extremely large and is likely to cause lots of confusion for subsequent PRs~

~@kevinbird15 I would fork Zach's repo instead and replay your changes (looks like only this one notebook) on top of that~

Nevermind I was confused b/c I was looking at something else. My apologies, just ignore what I said. I am still not sure if this will cause problems though 🤷🏽

kevinbird15 commented 2 years ago

Yeah, my git skills kind of failed me on this... I couldn't figure out how to discard those changes that were in the fastai master branch and keep the changes that I made. I'm kind of hoping if we pull over the fastai commits periodically that it kind of works itself out

kevinbird15 commented 2 years ago

Alright, I think all of the changes are done. Just need @warner-benjamin's approval (or disapproval)

kevinbird15 commented 2 years ago

There are missing doc strings in methods, including methods like Mixup.before_batch which are not simply calling other methods.

I have added docstrings to everything, let me know if you want anything changed

Also, could you remove the warnings/errors committed in the mixup notebook?

I don't know how to do this without modifying code. I did create an issue here since fixing the issue will require a code change, I don't think I should do that in this repo. Is there another way to fix this?

Used

import warnings
warnings.filterwarnings("ignore")

to temporarily suppress warnings to build pretty html :)