-
Notifications
You must be signed in to change notification settings - Fork 6
Revised metrics proposal #15
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: master
Are you sure you want to change the base?
Changes from 12 commits
8c96695
531c195
66eab2f
cf8ae38
1d8987c
6d5c5e3
00e0a1a
5ce78ec
9061c70
5cd4e88
75e1d1d
aac662c
bf96d2b
416d427
fbb634f
77777ba
6d69fa9
13107f4
048e484
9ff4e91
5040649
210872e
3ba87aa
c13da58
13069d5
2e10c46
8296f3a
f755711
0f28fe3
2ea2037
41bb325
205c239
70d8811
96703c6
7521261
f9ed32c
091c47e
180370b
16983bf
acfacab
697eee0
f0cdb5c
4feb19f
5649f92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,368 @@ | ||
| # Updates for Metrics | ||
|
|
||
| This is an update for the existing metrics document, which is being left in place for now as a point of comparison. | ||
|
|
||
| ## Assumed data | ||
|
|
||
| In the following we assume that we have variables of the following form defined: | ||
|
|
||
| ```python | ||
| y_true = [0, 1, 0, 0, 1, 1, ...] | ||
| y_pred = [1, 0, 0, 1, 0, 1, ...] | ||
| A_sex = [ 'male', 'female', 'female', 'male', ...] | ||
| A_race = [ 'black', 'white', 'hispanic', 'black', ...] | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
| A = pd.DataFrame(np.transpose([A_sex, A_race]), columns=['Sex', 'Race']) | ||
|
|
||
| weights = [ 1, 2, 3, 2, 2, 1, ...] | ||
| ``` | ||
|
|
||
| We actually seek to be very agnostic as to the contents of the `y_true` and `y_pred` arrays. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agnostic in what way? I think the proposal later explains this, but it may be easier to follow if that's moved up here.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was meaning in exactly the same way as your suggested comment. Let me rephrase. |
||
| Meaning is imposed on them by the underlying metrics. | ||
|
|
||
| ## Basic Calls | ||
|
riedgar-ms marked this conversation as resolved.
|
||
|
|
||
| ### Existing Syntax | ||
|
|
||
| Our basic method is `group_summary()` | ||
|
|
||
| ```python | ||
| >>> result = flm.group_summary(skm.accuracy_score, y_true, y_pred, sensitive_features=A_sex) | ||
| >>> print(result) | ||
| {'overall': 0.4, 'by_group': {'male': 0.6536, 'female': 0.213}} | ||
| >>> print(type(result)) | ||
| <class 'sklearn.utils.Bunch'> | ||
| ``` | ||
| The `Bunch` is an object which can be accessed in two ways - either as a dictionary - `result['overall']` - or via properties named by the dictionary keys - `result.overall`. | ||
| Note that the `by_group` key accesses another `Bunch`. | ||
|
|
||
| We allow for sample weights (and other arguments which require slicing) via `indexed_params`, and passing through other arguments to the underlying metric function (in this case, `normalize`): | ||
| ```python | ||
| >>> flm.group_summary(skm.accuracy_score, y_true, y_pred, sensitive_features=A_sex, indexed_params=['sample_weight'], sample_weight=weights, normalize=False) | ||
| {'overall': 20, 'by_group': {'male': 60, 'female': 21}} | ||
| ``` | ||
|
|
||
| We also provide some wrappers for common metrics from SciKit-Learn: | ||
| ```python | ||
| >>> flm.accuracy_score_group_summary(y_true, y_pred, sensitive_features=A_sex) | ||
| {'overall': 0.4, 'by_group': {'male': 0.6536, 'female': 0.213}} | ||
| ``` | ||
|
|
||
| ### Proposed Change | ||
|
|
||
| We do not intend to change the API invoked by the user. | ||
| What will change is the return type. | ||
| Rather than a `Bunch`, we will return a `GroupedMetric` object, which can offer richer functionality. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure whether we're already at the stage of discussing naming, but since we decided to discard
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a great time to be discussing naming, I think..... I'll see if I can come up with a few possibilities. |
||
|
|
||
| At this basic level, there is only a slight change to the results seen by the user. | ||
| There are still properties `overall` and `by_groups`, with the same semantics. | ||
| However, the `by_groups` result is now a Pandas Series, and we also provide a `metric` property to record the name of the underlying metric: | ||
| ```python | ||
| >>> result = flm.group_summary(skm.accuracy_score, y_true, y_pred, sensitive_features=A_sex) | ||
| >>> result.metric | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
| "sklearn.metrics.accuracy_score" | ||
| >>> result.overall | ||
| 0.4 | ||
| >>> result.by_groups | ||
| Male 0.6536 | ||
| Female 0.2130 | ||
| Name: sklearn.metrics.accuracy_score dtype: float64 | ||
| >>> print(type(result.by_groups)) | ||
| <class 'pandas.core.series.Series'> | ||
| ``` | ||
| The `metric` property (and using it as the name of the Series) could prove troublesome. | ||
| This is because the fully qualified function name, as reconstructed from `__name__`, `__qualname` and `__module__` might not match the user's expectation. | ||
| For example: | ||
| ```python | ||
| >>> import sklearn.metrics as skm | ||
| >>> skm.accuracy_score.__name__ | ||
| 'accuracy_score' | ||
| >>> skm.accuracy_score.__qualname__ | ||
| 'accuracy_score' | ||
| >>> skm.accuracy_score.__module__ | ||
| 'sklearn.metrics._classification' | ||
| ``` | ||
| We are seeing some of the actual internal structure here of SciKit-Learn, and the user might not be expecting that. | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
|
|
||
| We would continue to provide convenience wrappers such as `accuracy_score_group_summary` for users, and support passing through arguments along with `indexed_params`. | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
| There is little advantage to the change at this point. | ||
| This will change in the next section. | ||
|
|
||
|
riedgar-ms marked this conversation as resolved.
|
||
| ## Obtaining Scalars | ||
|
|
||
| ### Existing Syntax | ||
|
|
||
| We provide methods for turning the `Bunch`es returned from `group_summary()` into scalars: | ||
| ```python | ||
| >>> difference_from_summary(result) | ||
| 0.4406 | ||
| >>> ratio_from_summary(result) | ||
| 0.3259 | ||
| >>> group_max_from_summary(result) | ||
| 0.6536 | ||
| >>> group_min_from_summary(result) | ||
| 0.2130 | ||
| ``` | ||
| We also provide wrappers such as `accuracy_score_difference()`, `accuracy_score_ratio()` and `accuracy_score_min()` for user convenience. | ||
|
|
||
| One point which these helpers lack (although it could be added) is the ability to select alternative values for measuring the difference and ratio. | ||
| For example, the user might not be interested in the difference between the maximum and minimum, but the difference from the overall value. | ||
| Or perhaps the difference from a particular group. | ||
|
|
||
| ### Proposed Change | ||
|
|
||
| The `GroupedMetric` object would have methods for calculating the required scalars. | ||
| First, let us consider the differences. | ||
|
|
||
| We would provide operations to calculate differences in various ways (all of these results are a Pandas Series): | ||
| ```python | ||
| >>> result.differences() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very confusing to me. Defaults to relative_to="max"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean you'd like to have no default at all or a different one than "max"?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The user can specify, but I believe doing relative to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our current behavior is that we are taking differences relative to the minimum not maximum (that's why all of our differences are positive). That's how I wrote the proposal here. In other words, the "relative to" part is whatever you subtract from all the group-level metric values. But maybe it's confusing? An alternative term could be "origin". Another idea would be to say "from" instead of "relative_to"? |
||
| Male 0.0 | ||
| Female 0.4406 | ||
| Name: TBD dtype: float64 | ||
| >>> result.differences(relative_to='min') | ||
| Male -0.4406 | ||
| Female 0.0 | ||
| Name: TBD dtype: float64 | ||
| >>> result.differences(relative_to='min', abs=True) | ||
| Male 0.4406 | ||
| Female 0.0 | ||
| Name: TBD dtype: float64 | ||
| >>> result.differences(relative_to='overall') | ||
| Male -0.2436 | ||
| Female 0.1870 | ||
| Name: TBD dtype: float64 | ||
| >>> result.differences(relative_to='overall', abs=True) | ||
| Male 0.2436 | ||
| Female 0.1870 | ||
| Name: TBD dtype: float64 | ||
| >>> result.differences(relative_to='group', group='Female', abs=True) | ||
| Male 0.4406 | ||
| Female 0.0 | ||
| Name: TBD dtype: float64 | ||
| ``` | ||
|
riedgar-ms marked this conversation as resolved.
|
||
| The arguments introduced so far for the `differences()` method: | ||
| - `relative_to=` to decide the common point for the differences. Possible values are `'max'` (the default), `'min'`, `'overall'` and `'group'` | ||
| - `group=` to select a group name, only when `relative_to` is set to `'group'`. Default is `None` | ||
| - `abs` to indicate whether to take the absolute value of each entry (defaults to false) | ||
|
|
||
| The user could then use the Pandas methods `max()` and `min()` to reduce these Series objects to scalars. | ||
| However, this will run into issues where the `relative_to` argument ends up pointing to either the maximum or minimum group, which will have a difference of zero. | ||
| That could then be the maximum or minimum value of the set of difference, but probably won't be what the user wants. | ||
|
|
||
| To address this case, we should add an extra argument `aggregate=` to the `differences()` method: | ||
| ```python | ||
| >>> result.differences(aggregate='max') | ||
| 0.4406 | ||
| >>> result.differences(relative_to='overall', aggregate='max') | ||
| 0.1870 | ||
| >>> result.differences(relative_to='overall', abs=True, aggregate='max') | ||
| 0.2436 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this different from the line above with no
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without |
||
| ``` | ||
| If `aggregate=None` (which would be the default), then the result is a Series, as shown above. | ||
|
|
||
| There would be a similar method called `ratios()` on the `GroupedMetric` object: | ||
| ```python | ||
| >>> result.ratios() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same thing, don't like the default at all :-(
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And the same from me: I'm happy to change if there's agreement that requiring the argument makes more sense. |
||
| Male 1.0 | ||
| Female 0.3259 | ||
| Name: TBD dtype: float64 | ||
| ``` | ||
| The `ratios()` method will take the following arguments: | ||
| - `relative_to=` similar to `differences()` | ||
| - `group=` similar to `differences()` | ||
| - `ratio_order=` determines how to build the ratio. Values are | ||
| - `sub_unity` to make larger value the denominator | ||
| - `super_unity` to make larger value the numerator | ||
| - `from_relative` to make the value specified by `relative_to=` the denominator | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. both this line and the next say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a bit worried about the complexity of the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. The change is between numerator and denominator.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a question of naming, or whether we need all the cases? I'm not overly happy with the names myself, but I do think that all four cases might be useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A bit of both, I guess... but I think you're right that you need all four cases. |
||
| - `to_relative` to make the value specified by `relative_to=` the numerator | ||
| - `aggregate=` similar to `differences()` | ||
|
|
||
| We would also provide the same wrappers such as `accuracy_score_difference()` but expose the extra arguments discussed here. | ||
| One question is whether the default aggregation should be `None` (to match the method), or whether it should default to scalar results similar to the existing methods. | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
|
|
||
| In the section on Conditional Metrics below, we shall discuss one extra optional argument for `differences()` and `ratios()`. | ||
|
|
||
| ## Intersections of Sensitive Features | ||
|
|
||
| ### Existing Syntax | ||
|
|
||
| Our current API does not support evaluating metrics on intersections of sensitive features (e.g. "black and female", "black and male", "white and female", "white and male"). | ||
| To achieve this, users currently need to write something along the lines of: | ||
| ```python | ||
| >>> A_combined = A['Sex'] + '-' + A['Race'] | ||
|
|
||
| >>> accuracy_score_group_summary(y_true, y_pred, sensitive_features=A_combined) | ||
| { 'overall': 0.4, by_groups : { 'Female-Black':0.4, 'Female-Hispanic':0.5, 'Female-White':0.5, 'Male-Black':0.5, 'Male-Hispanic': 0.6, 'Male-White':0.7 } } | ||
| ``` | ||
| This is unecessarily cumbersome. | ||
| It is also possible that some combinations might not appear in the data (especially as more sensitive features are combined), but identifying which ones were not represented in the dataset would be tedious. | ||
|
|
||
|
|
||
| ### Proposed Change | ||
|
|
||
| If `sensitive_features=` is a DataFrame (or list of Series.... exact supported types are TBD), we can generate our results in terms of a MultiIndex. Using the `A` DataFrame defined above, a user might write: | ||
| ```python | ||
| >>> result = group_summary(skm.accuracy_score, y_true, y_pred, sensitive_features=A) | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
| >>> result.by_groups | ||
| Sex Race | ||
| Male Black 0.5 | ||
| White 0.7 | ||
| Hispanic 0.6 | ||
| Female Black 0.4 | ||
| White 0.5 | ||
| Hispanic 0.5 | ||
| Name: sklearn.metrics.accuracy_score, dtype: float64 | ||
| ``` | ||
| If a particular combination of sensitive features had no representatives, then we would return `None` for that entry in the Series. | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
|
|
||
| The `differences()` and `ratio()` methods would act on this Series as before. | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
|
|
||
| ## Conditional (or Segmented) Metrics | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
|
|
||
| For our purposes, Conditional Metrics (alternatively known as Segmented Metrics) do not return single values when aggregation is requested in a call to `differences()` or `ratios()` but instead provide one result for each unique value of the specified condition feature(s). | ||
|
|
||
| ### Existing Syntax | ||
|
|
||
| Not supported. | ||
| Users would have to devise the required code themselves | ||
|
|
||
| ### Proposed Change | ||
|
|
||
| We propose adding an extra argument to `differences()` and `ratios()`, to provide a `condition_on=` argument. | ||
|
|
||
| Suppose we have a DataFrame, `A_3` with three sensitive features: Sex, Race and Income Band (the latter having values 'Low' and 'High'). | ||
| This could represent a loan scenario where discrimination based on income is allowed, but within the income bands, other sensitive groups must be treated equally. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure I would call it discrimination based on income... perhaps making decisions based on income?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or to use abstract names and avoid mentioning
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been killing off 'sex' and 'gender' but I think that this does need a concrete example, or it will get too abstract. I have adjusted to 'decisions based on income' though since I agree that's a bit better. |
||
| When `differences()` is invoked with `condition_on=`, the result will not be a scalar, but a Series. | ||
| A user might make calls: | ||
| ```python | ||
| >>> result = accuracy_score_group_summary(y_true, y_test, sensitive_features=A_3) | ||
| >>> result.differences(aggregate=min, condition_on='Income Band') | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
| Income Band | ||
| Low 0.3 | ||
| High 0.4 | ||
| Name: TBD, dtype: float64 | ||
| ``` | ||
| We can also allow `condition_on=` to be a list of names: | ||
| ```python | ||
| >>> result.differences(aggregate=min, condition_on=['Income Band', 'Sex']) | ||
| Income Band Sex | ||
| Low Female 0.3 | ||
| Low Male 0.35 | ||
| High Female 0.4 | ||
| High Male 0.5 | ||
| ``` | ||
|
|
||
| ## Multiple Metrics | ||
|
|
||
| Finally, we can also allow for the evaluation of multiple metrics at once. | ||
|
|
||
| ### Existing Syntax | ||
|
|
||
| This is not supported. | ||
| Users would have to devise their own method | ||
|
|
||
| ### Proposed Change | ||
|
|
||
| We allow a list of metric functions in the call to group summary. | ||
| Results become DataFrames, with one column for each metric: | ||
| ```python | ||
| >>> result = group_summary([skm.accuracy_score, skm.precision_score], y_true, y_pred, sensitive_features=A_sex) | ||
| >>> result.overall | ||
| sklearn.metrics.accuracy_score sklearn.metrics.precision_score | ||
| 0 0.3 0.5 | ||
| >>> result.by_groups | ||
| sklearn.metrics.accuracy_score sklearn.metrics.precision_score | ||
| 'Female' 0.4 0.7 | ||
| 'Male' 0.6 0.75 | ||
| ``` | ||
| This should generalise to the other methods described above. | ||
|
|
||
| One open question is how extra arguments should be passed to the individual metric functions, including how to handle the `indexed_params=`. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can look at https://scikit-learn-enhancement-proposals.readthedocs.io/en/latest/slep006/proposal.html and scikit-learn/scikit-learn#16079 to get some ideas.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm reading that correctly, we would ditch the |
||
| On possible solution is to have lists, with indices corresponding to the list of functions supplied to `group_summary()` | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
| For example, for `index_params=` we would have: | ||
| ```python | ||
| indexed_params = [['sample_weight'], ['sample_weight']] | ||
| ``` | ||
| In the `**kwargs` a single `extra_args=` argument would be accepted (although not required), which would contain the individual `**kwargs` for each metric: | ||
| ```python | ||
| extra_args = [ | ||
| { | ||
| 'sample_weight': [1,2,1,1,3, ...], | ||
| 'normalize': False | ||
| }, | ||
| { | ||
| 'sample_weight': [1,2,1,1,3, ... ], | ||
| 'pos_label' = 'Granted' | ||
| } | ||
| ] | ||
| ``` | ||
| If users had a lot of functions with a lot of custom arguments, this could get error-prone and difficult to debug. | ||
|
|
||
| ## User Conveniences | ||
|
|
||
| We can consider allowing the metric function given to `group_summary()` to be represented by a string. | ||
| We would provide a mapping of strings to suitable functions. | ||
| This would make the following all equivalent: | ||
| ```python | ||
| >>> r1 = group_summary(sklearn.accuracy_score, y_true, y_pred, sensitive_features=A_sex) | ||
| >>> r2 = group_summary('accuracy_score', y_true, y_pred, sensitive_features=A_sex) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I presume this maps to the sklearn metrics? What if I want to define my own metric? Will that also work (although not with the string, of course)?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me add an 'in addition' to the introductory sentence. |
||
| >>> r3 = accuracy_score_group_summary( y_true, y_pred, sensitive_features=A_sex) | ||
| ``` | ||
| We would also allow mixtures of strings and functions in the multiple metric case. | ||
|
|
||
| ## Generality | ||
|
|
||
| Throughout this document, we have been describing the case of classification metrics. | ||
| However, we do not actually require this. | ||
| It is the underlying metric function which gives meaning to the `y_true` and `y_pred` lists. | ||
| So long as these are of equal length (and equal in length to the sensitive feature list - which _will_ be treated as a categorical), then `group_summary()` does not actually care about their datatypes. | ||
| For example, each entry in `y_pred` could be a dictionary of predicted classes and accompanying probabilities. | ||
| Or the user might be working on a regression problem, and both `y_true` and `y_pred` would be floating point numbers (or `y_pred` might even be a tuple of predicted value and error). | ||
| So long as the underlying metric understands the datastructures, `group_summary()` will not care. | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
|
|
||
| There will be an effect on the `GroupedMetric` result object. | ||
| Although the `overall` and `by_groups` properties will work fine, the `differences()` and `ratios()` methods may not. | ||
| After all, what does "take the ratio of two confusion matrices" even mean? | ||
|
riedgar-ms marked this conversation as resolved.
|
||
| We should try to trap these cases, and throw a meaningful exception (rather than propagating whatever exception happens to emerge from the underlying libraries). | ||
| Since we know that `differences()` and `ratios()` will only work when the metric has produced scalar results, this should be a straightforward test. | ||
|
|
||
| ## Pitfalls | ||
|
|
||
| There are some potential pitfalls which could trap the unwary. | ||
|
|
||
| The biggest of these are related to missing classes in the subgroups. | ||
| To take an extreme case, suppose that males were always being predicted classes A or B, while females were always predicted classes C or D. | ||
| The user could request precision scores, but the results would not really be comparable between the two groups. | ||
| With intersections of sensitive features, cases like this become more likely. | ||
|
|
||
| Metrics in SciKit-Learn usually have arguments such as `pos_label=` and `labels=` to allow the user to specify the expected labels, and adjust their behaviour accordingly. | ||
| However, we do not require that users stick to the metrics defined in SciKit-Learn. | ||
|
riedgar-ms marked this conversation as resolved.
|
||
|
|
||
| If we implement the convenience strings-for-functions piece mentioned above, then _when the user specifies one of those strings_ we can log warnings if the appropriate arguments (such as `labels=`) are not specified. | ||
| We could even generate the argument ourselves if the user does not specify it. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I probably would not include this in a first version, because it seems quite tricky. But I think we can add some sort of convenience function for this later on.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that it's going to be a thorny function to write (because it will need to be kept in sync with SciKit-Learn), and perhaps best deferred until later. |
||
| However, this risks tying Fairlearn to particular versions of SciKit-Learn. | ||
|
|
||
| Unfortunately, the generality of `group_summary()` means that we cannot solve this for the user. | ||
| It cannot even tell if it is evaluating a classification or regression problem. | ||
|
|
||
| ## The Wrapper Functions | ||
|
|
||
| In the above, we have assumed that we will provide both `group_summary()` and wrappers such as `accuracy_score_group_summary()`, `accuracy_score_difference()`, `accuracy_score_ratio()` and `accuracys_score_group_min()`. Do these wrappers add value, or do they end up just polluting our namespace and confusing users? | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
|
|
||
| The wrappers such as `demographic_parity_difference()` and `equalized_odds_difference()` are arguably useful, since they are specific metrics used in the literature (although even then we might want to add the extra `relative_to=` and `group=` arguments). | ||
| The case for `accuracy_score_group_summary()` and related functions is less clear. | ||
|
|
||
| ## Methods or Functions | ||
|
|
||
| Since the `GroupMetric` object contains no private members, it is not clear that it needs to be its own oject. | ||
|
riedgar-ms marked this conversation as resolved.
Outdated
|
||
| We could continue to use a `Bunch` but make the `group_by` entry/property return a Pandas Series (which would embed all the other information we might need). | ||
| In the multiple metric case, we would still return a single `Bunch` but the properties would both be DataFrames. | ||
|
|
||
| The question is whether users prefer: | ||
| ```python | ||
| >>> diff = group_summary(skm.recall_score, y_true, y_pred, sensitive_features=A).difference(aggregate='max') | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Obviously I'm just one data point, but I strongly prefer this over the version below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed - people were leaning that way, but since there's always the question of 'Do we really need a new concept?' I just wanted to call this out. |
||
| ``` | ||
| or | ||
| ```python | ||
| >>> diff = difference(group_summary(skm.recall_score, y_true, y_pred, sensitive_features=A), aggregate='max') | ||
| ``` | ||
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 we're dealing with metrics, should we separate floating point predictions (
decision_functionorpredict) from discrete ones? (assuming some metrics need soft values and some work with hard values)Maybe simply add :
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 don't think that we want separate
group_summary()functions for different types of underlying metrics, but I agree about the need to highlight that we just slice up they_predlist regardless of the types of its items, if that's what you meant. I was trying to write that below, but I'll make it more explicit.