Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ENH added agg argument to equalized odds difference and ratio to support "average odds" #960
base: main
Are you sure you want to change the base?
ENH added agg argument to equalized odds difference and ratio to support "average odds" #960
Changes from 1 commit
1aadaf3
d135261
997354a
991aaeb
cce6224
fa8dd6b
7a666c3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm fine including the two metrics--the goal and even behavior is very similar to our existing
equalized_odds_difference
andequalized_odds_ratio
, in the sense thataverage_odds_difference==0
oraverage_odds_ratio==1
indicate that there is no disparity according to the equalized odds criterion.But I think that the name is confusing for two reasons:
So my suggestion would be not to create a new function, but instead add a new optional argument to our existing
equalized_odds_difference
andequalized_odds_ratio
. For example, we could add an optional parameter calledaggregation_method
with its default value beingworst_case
, and a new possible value beingaverage
. Theaggregation_method
would just specify what's happening with the FPR_difference and FNR_difference -- whether we are taking the worst case or average.Thoughts?
[tagging @romanlutz @hildeweerts]
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'm not too bothered by the name wrt normal usage of the word "odds" - I think we can blame the person who came up with equalized odds for that. I am very surprised that AIF360 would allow for "cancelling out" the differences... isn't that the whole point of considering the FPR and TPR separately instead of looking at overall accuracy?
I like the proposed solution @MiroDudik - in particular the usage of
worst_case
would result in less mistakes compared tomin
andmax
, I think.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.
Also, if folks are in favor of just adding an optional argument to
equalized_odds_{difference,ratio}
, I'd be open to having something less verbose, e.g., we could follow pandas conventions and useagg={worst_case,mean}
.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.
Given the number of thumbs up I think we can assume that we can move forward with @MiroDudik 's suggestion?
@IanEisenberg would you like to implement the suggestion? If you don't have the time/interest that is of course perfectly fine (in that case we will just open a new issue).
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'm happy to implement Miro's suggestion. I'll get to it ASAP!
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.
Awesome, @IanEisenberg! Feel free to ask questions if anything is unclear.
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 @IanEisenberg couldn't continue on this I've picked it up today and continued along the lines discussed in this thread. Let me know if you have any concerns! @fairlearn/fairlearn-maintainers