classifier networks + trainer + tests

[adrienju]

Hello victor !

J'ai apporté les modifs sur ma première pull request, quand c'est comme ça tu préfères que j'en refasse une ou que j'update à la main sur la PR originale ?

Pour la loss (conversation gautier et yoshua), il semble que la convergence est meilleure quand tu utilises une l2 plutôt qu'une Cross-entropy. Si on utilise softmax + l2, il semble aussi qu'il est préférable de définir les labels de la façon suivante :

Entrainement du classifier :

Ground truth pour une image réel = (1, 0), pour une image sim = (0, 1)

Entrainement du générateur :

Ground truth pour une image réel = (0.5, 0.5) et pour une image sim = (0.5, 0.5) et pas (0, 1), (1, 0).

Point de débat avec gautier : doit-on mélanger dans un batch d'entrainement du classifier des données sim et réel afin de fournir un meilleur signal d'entrée à celui-ci ou peut-on l’entraîner séquentiellement comme c'est le cas pour le moment ?

J'ai lancé des tests pour voir ça je te fais un retour quand j'ai les résultats.

Bonne journée !

Le sam. 14 déc. 2019 à 06:24, Victor Schmidt notifications@github.com a écrit :

@vict0rsch requested changes on this pull request.

Cool! ça m'a l'air tout bon à quelques petits détails près que j'ai mis en commentaire ; merci!

In omnigan/classifier.py https://github.com/cc-ai/omnigan/pull/3#discussion_r357911699:

• self.max_pool2 = nn.MaxPool2d(2)

• self.BasicBlock2 = BasicBlock(int(self.channels / 2), int(self.channels / 4), True)

• self.avg_pool = nn.AvgPool2d((int(self.feature_size / 4), int(self.feature_size / 4)))

• self.fc = nn.Linear(int(self.channels / 4), 4)

• self.output_dim = dim

• def forward(self, x):

• max_pooled1 = self.max_pool1(x)

• res_block1 = self.BasicBlock1(max_pooled1)

• max_pooled2 = self.max_pool2(res_block1)

• res_block2 = self.BasicBlock2(max_pooled2)

• avg_pool = self.avg_pool(res_block2)

• fc_output = self.fc(avg_pool.squeeze())

• logits = nn.functional.softmax(fc_output)

• return logits

• return fc_output

puisque t'as pas besoin des self.max_pool1, self.BasicBlock1 etc. ailleurs que dans forward tu pourrais mettre tout ça dans un nn.Sequential non?

genre

self.model = nn.Sequential(*[

nn.MaxPool2d(2),

BasicBlock(self.channels, int(self.channels / 2), True),

...

])

et comme ça t'as qu'à faire

def forward(self, x):

return self.model(x)

---

In omnigan/classifier.py https://github.com/cc-ai/omnigan/pull/3#discussion_r357911776:

• self.fc = nn.Linear(int(self.channels / 4), 4)

• self.output_dim = dim

• def forward(self, x):

• max_pooled1 = self.max_pool1(x)

• res_block1 = self.BasicBlock1(max_pooled1)

• max_pooled2 = self.max_pool2(res_block1)

• res_block2 = self.BasicBlock2(max_pooled2)

• avg_pool = self.avg_pool(res_block2)

• fc_output = self.fc(avg_pool.squeeze())

• logits = nn.functional.softmax(fc_output)

• return logits

• return fc_output

+class BasicBlock(nn.Module):

Tu peux ajouter la source? genre juste un commentaire du type

github.com/user/repo/tree/master/folder/network/blocks.py

---

In omnigan/trainer.py https://github.com/cc-ai/omnigan/pull/3#discussion_r357911812:

@@ -96,7 +96,7 @@ def set_losses(self):

     }

     """

     self.losses = {"G": {"gan": {}, "cycle": {}, "tasks": {}}, "D": {}, "C": {}}

-

• ?

question ou typo?

In omnigan/utils.py https://github.com/cc-ai/omnigan/pull/3#discussion_r357911888:

• domain (list(str)): each element of the list should be in {rf, rn, sf, sn}

• Raises:

• ValueError: One of the domains listed is not in {rf, rn, sf, sn}

• Returns:

• torch.Tensor: 1D tensor mapping a domain to an int (not 1-hot)

• """

• mapping = {"rf": 2, "rn": 3, "sf": 0, "sn": 1}

• if not all(domain in mapping for domain in domains):

• raise ValueError(

• "Unknown domains {} should be in {}".format(domains, list(mapping.keys()))

• )

• return torch.tensor([mapping[domain] for domain in domains])

il manque une ligne vide à la fin du file il me semble, si tu formattes avec black il le fera tout seul je crois

In omnigan/trainer.py https://github.com/cc-ai/omnigan/pull/3#discussion_r357912094:

@@ -480,8 +479,43 @@ def get_translation_loss(self, multi_domain_batch):

 def update_d(self, batch):

     pass

• def update_c(self, batch):

• pass

• def update_c(self, multi_domain_batch):

• """

• Update the classifier using normal labels

• Args:

• multi_domain_batch (dict): dictionnary mapping domain names to batches from

• the trainer's loaders

• """

• self.c_opt.zero_grad()

• c_loss = self.get_classifier_loss(multi_domain_batch)

• ? Log policy

encore en suspend a prirori c'est comme ça que je l'aurais loggée mais on va voir ce qu'on fait de ce truc là après :p

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cc-ai/omnigan/pull/3?email_source=notifications&email_token=AIANHWLZMSI4AZ4SMTVYBWTQYS665A5CNFSM4J2F2ID2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPGRWEI#pullrequestreview-332208913, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIANHWNDIRTQSHEIE4ZUX6TQYS665ANCNFSM4J2F2IDQ .

[vict0rsch]

@adrienju oui le mieux est de patcher la PR en continuant à pusher sur cette branche pour pouvoir continuer une discussion unique IMHO

Et aussi si tu peux juste répondre un truc, ne serait-ce qu'un pouce ou un ok sur les commentaires si-dessus ça me permet de savoir que t'as vu, et tu resolve conversation quand t'as patché le truc ou qu'on est d'accord

[adrienju]

Adding L1 and L2 losses + tests + classifier network. @vict0rsch I need your opinion on trainer.py line 139, we need a way to assign the right loss according to the yaml. Best way to it ?

[adrienju]

Ready to merge after @vict0rsch's review

[vict0rsch]

Adding L1 and L2 losses + tests + classifier network.

Cool thanks! still a couple of questions before it's done but we're almost there :)

@vict0rsch I need your opinion on trainer.py line 139, we need a way to assign the right loss according to the yaml. Best way to it ?

For now the way you've done it seems fine, we'll see if it needs refactoring in the long run but we can work with that

[vict0rsch]

In my opinion we want the domains to be encoded like this we when we are training the classifier with l1 or l2 (and this is what we have right now) :

rf => [1, 0, 0, 0]
rn => [0, 1, 0, 0]
sf => [0, 0, 1, 0]
sn => [0, 0, 0, 1]

But when we are training the generator we want :

rf => [0.25 , 0.25, 0.25, 0.25]
rn => [0.25 , 0.25, 0.25, 0.25]
sf => [0.25 , 0.25, 0.25, 0.25]
sn => [0.25 , 0.25, 0.25, 0.25]

So the classifier being completely unsure means it would output 0.25 everywhere since the output sum to 1 and we have 4 logits. Are you agree ?

I agree with your other possibility do you want to include this mode in the classifier as well ?

OK when you put it like this it makes sense and I agree : the output vector is a distribution and we want it to be uniform across domains. It's just, we could represent domains as a joint distribution and have dependent dimensions (1-hot and 0.25) or as 2 independent distributions (real or sim for 1 dimension, flooded or not for the orhter) and have 2 ones in the vectors and 0.5 when backproping through the gen.

But nevermind let's continue with 1-h and 0.25 I agree 👍