docment GAN by tmabraham · Pull Request #72 · fastaidocsprint/fastai

tmabraham · 2022-02-20T09:20:08Z

Okay here's my first docment PR, let me know if there are any issues and I would appreciate any feedback you have.

I'll note I did take out the after_epoch and show_img arguments since they weren't doing anything. I will take a little time to debug and get them working. Shall I make an issue in the main fastai repo? I can work on fixing that after the docment sprint.

review-notebook-app · 2022-02-20T09:20:12Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

tmabraham · 2022-02-20T09:22:37Z

This PR would finish #20.

warner-benjamin

As we discussed on discord, added suggestions for one of the methods style issues. If you could fix the highlighted formatting issues from basic_generator in other methods, everything looks pretty good.

tmabraham · 2022-02-21T02:06:34Z

Finished the changes based on @warner-benjamin's and @muellerzr's suggestions.

I will note this very weird issue we discussed on Discord voice channel.

In the code there is the following function:

#export
def generate_noise(
    fn, # Dummy argument so it works with `DataBlock`
    size=100 # Size of returned noise vector
) -> InvisibleTensor: 
    "Generate noise vector."
    return cast(torch.randn(size), InvisibleTensor)

I have added some docmenting but not type annotations. However, once I specifically add a type annotation to size, the full notebook doesn't run without error anymore:

#export
def generate_noise(
    fn, # Dummy argument so it works with `DataBlock`
    size:int=100 # Size of returned noise vector
) -> InvisibleTensor: 
    "Generate noise vector."
    return cast(torch.randn(size), InvisibleTensor)

What happens is that generate_noise is used as get_x for a GAN DataBlock. The DataBlock functionality will pass a Path to generate_noise but it's a dummy argument and will return the noise vector as our x which is what we want. But when I add the type annotation, the Path becomes the x, as I discovered through dblock.summary.

This is a little concerning if code breaks due to the type annotations, so as a side note, we should all be making sure the tests pass for all of these PRs (must be enabled for first-time contributors).

warner-benjamin

Noticed one formatting error. Made a couple suggestions to make the doc strings more concise. Please replicate those in the other doc strings. Also, I think the loss_crit doc string had an error, but am not 100% sure.

Co-authored-by: Benjamin Warner <me@benjaminwarner.dev>

…nto docment_gan

…int into docment_gan

tmabraham · 2022-02-26T03:15:23Z

@warner-benjamin updated, let me know if there's anything else that needs to be updated...

warner-benjamin

I don't think we need to add a None type hint for args with a default of None. I provided an example.

Remove the None type hints and it looks good.

kevinbird15 · 2022-03-07T04:31:55Z

@@ -121,23 +160,33 @@ def forward(self, output, target):



Add types to AdaptiveLoss

kevinbird15 · 2022-03-07T04:38:10Z

@@ -259,7 +316,12 @@ class InvisibleTensor(TensorBase):
    def show(self, ctx=None, **kwargs): return ctx


Maybe add doc string to explain InvisibleTensor. What is Invisible about it?

kevinbird15 · 2022-03-07T04:45:43Z

+        switch_eval:bool=False, # Whether the model should be set to eval mode when calculating loss
+        **kwargs
+    ):
+        "Create a WGAN from `dls`, `generator` and `critic`."


What is a WGAN? Is there a paper link or anything for this

warner-benjamin · 2022-03-10T23:46:09Z

One other thought, maybe the model training should be behind a nbdev #slow tag so we are not replicating it every time tests are run on GH Actions. Would also be preferred not to check in the UserWarnings to the documentation

Co-authored-by: Kevin Bird <kevinbird15@gmail.com>

tmabraham · 2022-03-13T07:53:53Z

One other thought, maybe the model training should be behind a nbdev #slow tag so we are not replicating it every time tests are run on GH Actions. Would also be preferred not to check in the UserWarnings to the documentation

There is an all_slow tag at the top of the notebook so that's not needed. IDK about the UserWarnings though. Technically, there is a fastai PR to fix this which we can resolve after this sprint.

warner-benjamin · 2022-03-13T17:30:37Z

Alright. Resolve the CI errors and then it looks good to me.

tmabraham · 2022-03-14T05:31:17Z

@warner-benjamin updated...

warner-benjamin

LGTM

docment GAN

d17796a

tmabraham assigned muellerzr Feb 20, 2022

tmabraham requested review from muellerzr and warner-benjamin February 20, 2022 18:02

warner-benjamin reviewed Feb 20, 2022

View reviewed changes

Comment thread fastai/vision/gan.py Outdated

warner-benjamin requested changes Feb 20, 2022

View reviewed changes

tmabraham added 2 commits February 21, 2022 01:46

make changes based on Benjamin's suggestions

86e017f

added back show_img stuf as Zach asked

79ba51c

tmabraham requested a review from warner-benjamin February 21, 2022 02:06

warner-benjamin requested changes Feb 23, 2022

View reviewed changes

Comment thread fastai/vision/gan.py

Comment thread fastai/vision/gan.py

Comment thread fastai/vision/gan.py Outdated

tmabraham and others added 8 commits February 25, 2022 18:26

Update fastai/vision/gan.py

5e12155

Co-authored-by: Benjamin Warner <me@benjaminwarner.dev>

Update fastai/vision/gan.py

06a6722

Co-authored-by: Benjamin Warner <me@benjaminwarner.dev>

Update fastai/vision/gan.py

b07c536

Co-authored-by: Benjamin Warner <me@benjaminwarner.dev>

Merge branch 'master' of github.com:muellerzr/fastai-docment-sprint i…

5d2606c

…nto docment_gan

Merge branch 'docment_gan' of github.com:muellerzr/fastai-docment-spr…

60afee8

…int into docment_gan

update nb

e1feb97

update doc

3cf5344

make docments more concise as suggested by Benjamin

aa2cd40

tmabraham requested a review from warner-benjamin February 26, 2022 03:15

warner-benjamin reviewed Feb 26, 2022

View reviewed changes

Comment thread fastai/vision/gan.py

kevinbird15 requested changes Mar 7, 2022

View reviewed changes

tmabraham and others added 4 commits March 12, 2022 22:42

Update fastai/vision/gan.py

af70393

Co-authored-by: Kevin Bird <kevinbird15@gmail.com>

Update fastai/vision/gan.py

628ccf5

Co-authored-by: Kevin Bird <kevinbird15@gmail.com>

Update fastai/vision/gan.py

8d8c52d

Co-authored-by: Kevin Bird <kevinbird15@gmail.com>

Update fastai/vision/gan.py

f8bc518

Co-authored-by: Kevin Bird <kevinbird15@gmail.com>

Update fastai/vision/gan.py

78d01b5

Co-authored-by: Kevin Bird <kevinbird15@gmail.com>

incorporate the rest of @kevinbird15's suggestions

1bca111

tmabraham requested review from kevinbird15 and warner-benjamin March 14, 2022 05:30

warner-benjamin approved these changes Mar 14, 2022

View reviewed changes

warner-benjamin merged commit 9f21d3f into master Mar 14, 2022

		@@ -259,7 +316,12 @@ class InvisibleTensor(TensorBase):
		def show(self, ctx=None, **kwargs): return ctx

Conversation

tmabraham commented Feb 20, 2022

Uh oh!

review-notebook-app Bot commented Feb 20, 2022

Uh oh!

tmabraham commented Feb 20, 2022

Uh oh!

Uh oh!

warner-benjamin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmabraham commented Feb 21, 2022

Uh oh!

warner-benjamin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tmabraham commented Feb 26, 2022

Uh oh!

warner-benjamin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinbird15 Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinbird15 Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinbird15 Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

warner-benjamin commented Mar 10, 2022

Uh oh!

tmabraham commented Mar 13, 2022

Uh oh!

warner-benjamin commented Mar 13, 2022

Uh oh!

tmabraham commented Mar 14, 2022

Uh oh!

warner-benjamin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

warner-benjamin left a comment •

edited

Loading