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

Separate channel configs for 2 G's and 2 D's for CycleGAN #85

Merged
merged 23 commits into from
Aug 20, 2021
Merged

Conversation

cnmy-ro
Copy link
Collaborator

@cnmy-ro cnmy-ro commented Apr 11, 2021

No description provided.

@cnmy-ro cnmy-ro linked an issue Apr 12, 2021 that may be closed by this pull request
@cnmy-ro
Copy link
Collaborator Author

cnmy-ro commented May 5, 2021

Made some very minor but important changes in this latest commit, other than my HX4 stuff. For example:

  1. midaGAN/engines/validator_tester.py: First, in the denormalize block starting from line 63, now we give clones of tensors to the denormalize() function because otherwise, it changes the visuals dict data and messes up the wandb visualization. Second, change in visuals dictionary key names to match those in GAN classes. For example, visuals['A'] is changed to visuals['real_A'].
  2. midaGAN/utils/trackers/utils.py: A minor change in the _split_multimodal_visuals() function to also account for the masks
  3. midaGAN/nn/discriminators/patchgan/patchgan3d.py: Set the output channel to 1 in the last `Conv3d' layer
  4. midaGAN/nn/generators/unet/unet3d.py: In the config class, the datatype of use_dropout setting was missing. Set it to bool

EDIT: Bugs (3) and (4) were also fixed by @surajpaib in PR #90 and merged into master by @ibro45

@ibro45
Copy link
Collaborator

ibro45 commented May 6, 2021

great stuff, i would merge it right away, but won't atm because of the separate in_channels config, need to figure out how to make it better as we discussed a while ago




class UnpairedPatchSampler3D():
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we make both the unpaired and paired patchsampler part of the framework? Maybe have a base patch sampler that has the main functionalities and then write Unpaired and Paired on top of them? Or smth like that

Copy link
Collaborator Author

@cnmy-ro cnmy-ro Aug 14, 2021

Choose a reason for hiding this comment

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

Sounds good! I'm in favour as well of having a base sampler.

And what do you think about this probability map-based system? The default one is the uniform-random sampling scheme and I added some specialized stuff like fdg-pet-weighted sampling. We could give each of them an integer code like 1, 2 and so on. And maybe also an option for the user to supply their custom sampling map.
EDIT: @surajpaib did create a separate issue for this one #89

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah true, yes, i agree with everything. once we merge this pull request we could do that if you agree. we'd just have to give more general names than fdg-pet-weighted, if that makes sense of course, rn i don't know what it is. let's keep the discussion in that issue then.

@ibro45 ibro45 closed this Aug 14, 2021
@ibro45 ibro45 mentioned this pull request Aug 14, 2021
@ibro45 ibro45 reopened this Aug 14, 2021
@ibro45
Copy link
Collaborator

ibro45 commented Aug 17, 2021

@cnmy-ro i redesigned how the channels are separated.

For generator:

generator:  
            name: "Resnet2D"
            n_residual_blocks: 9
            in_out_channels:
                AB: [3, 3]
                BA: [3, 3] # Optional, it's interpolated from AB by default

Similar thing for discriminator.

Right now it's defined like that, as a dict, but in the future, when OmegaConf allows Union, we'll allow either like that, as a dist with keys for direction/domain, or just a single tuple (generator) or int (discriminator) when there is only one network or when both networks should have the same value. Added TODOs in the code explaining that. OmegaConf should enable it in the next version, 2.2, but that's probably months away.

Do you agree with what I did? Could you give it a run? We should also update all the yamls then.

It's not ideal the way it is now, i don't find it super beginner-friendly that it exposes channel separation so much, coz you only need it in rare situations. But when OmegaConf will allow Unions then it will be good.

@cnmy-ro
Copy link
Collaborator Author

cnmy-ro commented Aug 19, 2021

Yeah, I agree with this. It's really much cleaner this way compared to how I defined these before. I'll run some tests then.

@ibro45
Copy link
Collaborator

ibro45 commented Aug 20, 2021

thanks! merging. if you update yamls or something feel free to push directly to master

@ibro45 ibro45 merged commit dcc641a into master Aug 20, 2021
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.

Make CycleGAN's separate channel config cleaner and more readable
2 participants