Skip to content
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] merge metric files into _base_metrics.py, rename _disparities.py to fairness_metrics.py, and move _input_manipulations.py to the utils module #1202

Merged
merged 15 commits into from Feb 7, 2023

Conversation

romanlutz
Copy link
Member

Description

closes #455

This PR merges metric files into _base_metrics.py, renames _disparities.py to _fairness.py, and moves _input_manipulations.py to the utils module. None of the contents of the migrated functions was changed.

Tests

  • no new tests required
  • new tests added
  • existing tests adjusted

Documentation

  • no documentation changes needed
  • user guide added or updated
  • API docs added or updated
  • example notebook added or updated

Screenshots

@hildeweerts
Copy link
Contributor

hildeweerts commented Feb 6, 2023

Other-ml tests seem to have failed due to a deprecation error in xgboost: "UserWarning: use_label_encoder is deprecated in 1.7.0." According to the deprecation tracker users need to do label encoding manually. We set it to False so we just need to remove use_label_encoder=False from our code.

EDIT: nvm I see you've already opened #1204 to fix this

Copy link
Contributor

@hildeweerts hildeweerts left a comment

Choose a reason for hiding this comment

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

Thanks, @romanlutz! The renaming makes things so much clearer., IMO. There are a few codecov warnings we should look into, other than that this looks good to me!

@riedgar-ms
Copy link
Member

When it comes to naming, I thought we preferred disparity to fairness on the grounds that the former was definitely measurable, whereas the latter was not?

@romanlutz
Copy link
Member Author

When it comes to naming, I thought we preferred disparity to fairness on the grounds that the former was definitely measurable, whereas the latter was not?

I'm following the instructions from the issue #455 🙂 I don't care either way tbh. It's just a file name 🙂 @hildeweerts do you have thoughts?

@hildeweerts
Copy link
Contributor

Given that we refer to this type of metric as fairness metric a lot throughout the user guide I think using "fairness metric" is perfectly sensible :)

@romanlutz
Copy link
Member Author

There are a few codecov warnings we should look into, other than that this looks good to me!

I didn't change any code, so codecov is probably angry that file coverage went down or something like that.

Actually, looking at the specific files it points out... I didn't touch those or anything related at all. Must be a codecov bug?

@hildeweerts
Copy link
Contributor

Actually, looking at the specific files it points out... I didn't touch those or anything related at all. Must be a codecov bug?

I think these files probably predate our installment of codecov, so maybe it's not a bug but an actual lack of coverage? In any case I don't think it is something that needs to be fixed in this PR.

@hildeweerts hildeweerts self-requested a review February 6, 2023 19:13
@riedgar-ms
Copy link
Member

I've no objection to the consolidation and reorganisation of files, but as I recall, calling them 'disparity metrics' was a pretty deliberate choice. That said, it's not user facing either.

@MiroDudik
Copy link
Member

My sense is that we have since departed from calling them "disparity metrics". I'm more comfortable calling them "fairness metrics"... To be parallel with _base_metrics.py, maybe we should call the old _disparities.py something like _fairness_metrics.py?

@riedgar-ms
Copy link
Member

OK, I'm fine with the renaming then.

@romanlutz romanlutz changed the title [ENH] merge metric files into _base_metrics.py, rename _disparities.py to fairness.py, and move _input_manipulations.py to the utils module [ENH] merge metric files into _base_metrics.py, rename _disparities.py to fairness_metrics.py, and move _input_manipulations.py to the utils module Feb 7, 2023
@romanlutz romanlutz merged commit 5d00cf9 into fairlearn:main Feb 7, 2023
@romanlutz romanlutz deleted the romanlutz/merge_metric_files branch February 7, 2023 19:28
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.

Improve organization of metrics module
4 participants