-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Conversation
…s/adversarial-bias-mitigation
…s/adversarial-bias-mitigation
I have misspelled "adversarial" all my life 🤦🏻 . Seems like autocorrect bailed me out a lot. |
!!! Note: | ||
Intended to be used with `AdversarialBiasMitigator`. | ||
trainer.model is expected to have `predictor` and `adversary` data members. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could, if you wanted to, put in a check for this condition and throw an exception if it isn't met.
} | ||
|
||
trainer.model.predictor.zero_grad() | ||
batch_outputs["loss"].backward() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If "loss"
and "adversary_loss"
don't use exactly the same computation graph, does that mean that parts of the computation graph of "adversary_loss"
could stick around when we don't want them to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a really good point about part of the computation graph not getting erased! Upon further reading, it looks like the computation graph will stay around until adversary_loss
goes out of scope. So I added this in the callback to remove all references to the adversary_loss
in the graph and instead keep a view of the loss that's not in the graph:
# remove adversary_loss from computation graph
batch_outputs["adversary_loss"] = batch_outputs["adversary_loss"].detach()
allennlp/training/optimizers.py
Outdated
@@ -278,6 +278,11 @@ def __init__( | |||
" Alternatively, you can remove this optimizer from the provided `optimizers`" | |||
" if it is not relevant to a particular parameter group." | |||
) | |||
# default optimizer is required, but may not be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"must not be used" or "using it is optional"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what you mean from context, but it would be easier to read if worded unambiguously.
of some protected variable Z. Informally, "knowing Y would not help | ||
you predict Z any better than chance" (Zaldivar et al., 2018). This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be the other way round? "knowing Z would not help you predict Y"? Or is it stating that knowing the outcome shouldn't give you the information about the protected variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter, it's stating that knowing the outcome shouldn't give you information about the protected variable.
|
||
vocab : `Vocabulary` | ||
Vocabulary of predictor. | ||
predictor : `Model` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not strictly an issue. We use the term predictor
differently in the library elsewhere; should we change the name here? If this is adhering to the paper's terminology, it's probably okay to keep it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm adhering to the paper's terminology.
Changes/additions proposed in this pull request:
AdversarialBiasMitigator
, a Model wrapper to adversarially mitigate biases in predictions produced by a pretrained model for a downstream task. Tests are in: added AdversarialBiasMitigator tests and model allennlp-models#281.IndexOutOfBoundsException
inMultiOptimizer
when checking if optimizer received any parameters.MultiOptimizer
so that while a default optimizer is still required, an error is not thrown if the default optimizer receives no parameters.Depends on allenai/allennlp-models#281.