Skip to content
This repository was archived by the owner on Feb 7, 2025. It is now read-only.

475 add different downsampling methods to patch gan discriminator - #479

Merged
virginiafdez merged 8 commits into
Project-MONAI:mainfrom
StijnvWijn:475-add-different-downsampling-methods-to-PatchGAN-discriminator
Apr 3, 2024
Merged

virginiafdez merged 8 commits into
Project-MONAI:mainfrom
StijnvWijn:475-add-different-downsampling-methods-to-PatchGAN-discriminator

Conversation

@StijnvWijn

Copy link
Copy Markdown
Contributor

Added features according to issue #475 leaving the default behaviour the same. According to my testing it should be as fast as the previous implementation.

Additionally, I fixed the 2D_spade_VAE.py tutorial, as it didn't work on my Windows laptop.

@virginiafdez

Copy link
Copy Markdown
Contributor

@StijnvWijn thanks for fixing the tutorial. Since the tutorial file has undergone a lot of changes, could you create a different issue and PR for this? It's a different issue from the one treated in this one, so we would like to treat them separately.
Just copy-pasting the tutorial of this branch into the new PR would suffice.
Thanks.

@virginiafdez

Copy link
Copy Markdown
Contributor

In addition, could you add use cases to test test_patch_gan.py that uses the available pooling methods? No additional test is needed, just additional configurations in TEST_3D and TEST_2D.

@virginiafdez virginiafdez left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for addressing this PR to enable having pooling layers in the Multi-scale PatchGAN discriminator. Prior to approve the PR, the following changes need to be performed on the code:

  1. No support for 3D; pooling layers should be defined using get_pool_layer from monai.networks.layers, to take in spatial_dims.
  2. Due to not using this function, the padding behaviour is wrong. The default should be "same" padding. This will probably get fixed addressing the previous point.
  3. At the moment, setting the pooling layers as an attribute is causing the forward method not to work well. It should not be set as an attribute.
  4. Use cases should be added to test_patch_gan.py to make sure the codebase works with pooling_method != None.
  5. The changes to the SPADE tutorial to make it work on Windows should be a separate PR as it is an entirely different issue.

Comment thread generative/networks/nets/patchgan_discriminator.py Outdated
Comment thread generative/networks/nets/patchgan_discriminator.py Outdated
Comment thread generative/networks/nets/patchgan_discriminator.py
Comment thread generative/networks/nets/patchgan_discriminator.py Outdated
@StijnvWijn
StijnvWijn force-pushed the 475-add-different-downsampling-methods-to-PatchGAN-discriminator branch from c1e1225 to 7a90fb8 Compare March 26, 2024 16:40
@StijnvWijn
StijnvWijn requested a review from virginiafdez March 27, 2024 14:10

@virginiafdez virginiafdez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hello,

Thanks very much for applying all the changes! Everything looks good now! The only thing is adding the padding on the pooling layer creation because apparently, it's 0 by default.

Once this is added, we can approve the PR!

Comment thread generative/networks/nets/patchgan_discriminator.py
@virginiafdez

Copy link
Copy Markdown
Contributor

Things work, I ran the autofix script and committed these changes.

@virginiafdez
virginiafdez self-requested a review April 3, 2024 12:31

@virginiafdez virginiafdez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ran autofix on top of the changes.

@virginiafdez
virginiafdez merged commit 4bc610a into Project-MONAI:main Apr 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants