Skip to content

Multi-level masking - #125

Open
51N84D wants to merge 8 commits into
masterfrom
multi_level_masking
Open

Multi-level masking#125
51N84D wants to merge 8 commits into
masterfrom
multi_level_masking

Conversation

@51N84D

@51N84D 51N84D commented Aug 24, 2020

Copy link
Copy Markdown
Member

Current approach just adds bit-conditioning to the latent vector, and determines which (simulated) ground truth mask to compute losses with.

@vict0rsch

Copy link
Copy Markdown
Contributor

Is that up to date? What's keeping us from merging?

@51N84D
51N84D marked this pull request as ready for review September 22, 2020 02:12
@51N84D
51N84D requested review from melisandeteng, tianyu-z and vict0rsch and removed request for melisandeteng September 22, 2020 02:12
@51N84D

51N84D commented Sep 22, 2020

Copy link
Copy Markdown
Member Author

Alright it's finally ready to review. Try running it before approving, just to make sure nothing is broken...

@vict0rsch vict0rsch 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.

Requesting changes because I'd like to go over some of the code liv with you @51N84D

Comment thread omnigan/discriminator.py
Comment thread omnigan/trainer.py
Comment thread omnigan/trainer.py Outdated
if "m2" in self.opts.tasks:
prediction = self.G.decoders[update_task](
torch.cat(
(self.z, self.label_1[0, :, :, :].unsqueeze(0)),

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.

idem

Comment thread omnigan/trainer.py Outdated
if update_task == "m2":
prediction = self.G.decoders["m"](
torch.cat(
(self.z, self.label_2[0, :, :, :].unsqueeze(0)),

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.

self.label_0[0, :, :, :].unsqueeze(0) should be the same as self.label_0[:1, ...]

Comment thread omnigan/trainer.py Outdated
task_saves.append(x * (1.0 - target.repeat(1, 3, 1, 1)))

elif update_task == "d":
if update_task == "d":

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.

why not elif ?

Comment thread omnigan/trainer.py

step_loss += update_loss

elif update_task == "m2":

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.

If I read this right, the only things that change between the if and elif are self.label_1[:, 0, 0, 0].squeeze() vs self.label_2[:, 0, 0, 0].squeeze() and self.logger.losses.generator.task_loss. I bet you can refactor this whole block in a much shorter way by having variables and a common code dependent on those. Cleaner, shorter, less error-prone (don't need to change 2 pieces of code if you change the logic) more versatile (what about more flood levels?)

Comment thread omnigan/trainer.py
self.D["m"]["Advent"],
)
if "m2" in self.opts.tasks:
# --------ADVENT LOSS---------------

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.

Add CoBlock to your vscode extension and use cmd+shift+k to make nice comment blocks instead of those atrocious imbalanced hybrid headers

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.

:p

@vict0rsch

Copy link
Copy Markdown
Contributor

How about this PR, is it ready @51N84D ?

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.

4 participants