-
Notifications
You must be signed in to change notification settings - Fork 889
Adding intersectional bias mitigation to AIF360 #538
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
base: main
Are you sure you want to change the base?
Conversation
According to pytest, our code does not reach the desired coverage of 80%. This happens because our code is multi threaded and it was not obvious to us how pytest can support such kind of code. Nonetheless, we have checked that all functions of the main algorithm file are called during our tests. |
Signed-off-by: Kalousios <[email protected]>
Signed-off-by: ckalousi <[email protected]>
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.
…bar support of the main algorithm added Signed-off-by: Kalousios <[email protected]>
Dear Rahul, we are very grateful for taking the time to review our code and offer some very valuable comments. We are also extremely sorry we couldn't address your comments earlier as we had to observe some critical deadlines in our work and also coordinate our actions regarding our pull request. About your comment on the init method. This is a great catch. Indeed self.model is not needed in the current setting. We had only put it there in case we wanted in the future to expand support of our Intersectional Fairness to more algorithms. Under current circumstances it makes sense to comment out this line (line 27 of your screenshot). You have also made some very valuable comments in your first version of your comment (before the edit). We considered all of them, and although they are all very important we could only use current resources to address some of them. More specifically, although it would be nice to switch to TensorFlow 2 for future compatibility, our code is based on the code of the Adversarial Debiasing algorithm found in AIF360 which in turn is based on TensorFlow 1. It is very difficult to modify our code to support TensorFlow 2 the time Adversarial Debiasing uses TensorFlow 1. If in the future the original algorithm is updated we would be happy to also update our code. Following one of your comments we have now implemented evaluation progress bars in our algorithm. Once again thank you for your time and we would be happy to further discuss any of your suggestions or concerns. Best regards, |
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.
Thanks for your patience here! My comments are mostly concerned with cleaning up the code and reducing redundancies but overall it's in pretty good shape. Good work!
aif360/algorithms/isf_helpers/isf_analysis/intersectional_bias.py
Outdated
Show resolved
Hide resolved
aif360/algorithms/isf_helpers/isf_analysis/intersectional_bias.py
Outdated
Show resolved
Hide resolved
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.
is there any way we can just use the AdversarialDebiasing
class directly instead of this wrapper? this doesn't seem to be doing much.
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.
Since isf needs to distinguish between pre/in/post processing, it needs a wrapper to abstract it. It would be hard to remove the wrapper classes.
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.
Can you explain more? Why can't we just compare the algorithm name? i.e., if self.algorithm == "AdversarialDebiasing": self.approach_type == "InProcessing"
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.
thank you very much for your comment, we have now deleted the wrapper class and use the AIF360 classes directly
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.
same as before -- can we get rid of these wrapper classes?
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.
same comment as the previous comment
Dear @hoffmansc thank you very much for your patience while reviewing our code. We agree to all of your comments and we have tried to address them to the best of our understanding. We are committed to continue working with you, until we reach the required standards for publication. |
Dear maintainers, we have now addressed the requested changes to the best of our understanding and we are looking forward to your further comments at your convenience. Thank you in advance. |
|
||
from tqdm import tqdm | ||
|
||
class IntersectionalFairness(): |
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.
can we have this extend aif360.algorithms.Transformer
?
class IntersectionalFairness(): | |
class IntersectionalFairness(Transformer): |
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.
indeed, thank you, we have now extended as suggested
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.
Can you explain more? Why can't we just compare the algorithm name? i.e., if self.algorithm == "AdversarialDebiasing": self.approach_type == "InProcessing"
wrapper class removed bugs fixed Signed-off-by: Kalousios <[email protected]>
Dear @hoffmansc, thank you very much for your latest review. We have now addressed all of your comments to the best of our understanding and we are waiting for further communication. We hope we are reaching closer to publication. |
@mnagired @hoffmansc
We have implemented an intersectional bias mitigation algorithm based on https://doi.org/10.1007/978-3-030-87687-6_5 (see also https://doi.org/10.48550/arXiv.2010.13494 for the arxiv version) as discussed further on issue #537. Additional details are available in the demo notebook.