Skip to content

Conversation

ckalousi
Copy link

@ckalousi ckalousi commented Aug 26, 2024

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

@ckalousi
Copy link
Author

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.

Copy link

@RahulVadisetty91 RahulVadisetty91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

In the init method, self.model is set to None, but there's no mechanism for initializing or assigning a model. While the abstract fit method may be responsible for setting the model, it's not clear how this works.

…bar support of the main algorithm added

Signed-off-by: Kalousios <[email protected]>
@ckalousi
Copy link
Author

ckalousi commented Nov 2, 2024

@RahulVadisetty91

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,
Chrysostomos

Copy link
Collaborator

@hoffmansc hoffmansc left a 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!

Copy link
Collaborator

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.

Copy link
Author

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.

Copy link
Collaborator

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"

Copy link
Author

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

Copy link
Collaborator

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?

Copy link
Author

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

@ckalousi
Copy link
Author

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.

@ckalousi ckalousi requested a review from hoffmansc April 1, 2025 09:57
@ckalousi
Copy link
Author

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():
Copy link
Collaborator

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?

Suggested change
class IntersectionalFairness():
class IntersectionalFairness(Transformer):

Copy link
Author

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

Copy link
Collaborator

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]>
@ckalousi ckalousi requested a review from hoffmansc September 15, 2025 18:46
@ckalousi
Copy link
Author

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.

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.

3 participants