Open tsirif opened 5 years ago
So this was supposed to be default behavior at some point, but it was removed. That said, the way losses and results are handled in the backend are quite messy, and probably need to be refactored. What about for adding losses, there is an "add_loss" method instead? Then all losses are available via a dictionary.
I have refactored lots of stuff to comply with all things intented (incl. isolating a routine's contribution to losses and results and reporting them prefixed to all_epoch_losses
- which I think is the original intention), without breaking the API for ModelPlugin
developers. I am going to make a separate PR for this as well after ICML.
Btw as of now I am familiar with the whole codebase
OK! Looking forward to the PR!
I have come to realize by the implementation in
built_ins/gan.py
that two separate calls are done in order to update the discriminator. The first optimizer update utilizes the gan loss and the second the gradient penalty.This is because LossHandle will overwrite (s1) any value for a specific network key, which is an inconvenient behaviour. I can see in s2 and s3 that there was an intention to implement a convenient behaviour, but it seems that it has not been done.
I propose the following, tell me what you think:
self.losses.network = a
will overwrite/set the loss for thenetwork
.self.losses.network += a
would add to the already existing loss, if it exists, and if it doesn't it sets it toa
.method
oradd_value=True
as it would only introduce confusion and incoherencies to the API for those creatingModelPlugin
s.