fairlearn icon indicating copy to clipboard operation
fairlearn copied to clipboard

DOC Validate and document scalar entry requirement for sensitive_features

Open Mariam-ke opened this issue 7 months ago • 9 comments

…contain scalar values

Description

This PR addresses part 1 and 2 of issue #1250 by ensuring that all entries within sensitive_features are scalar values (e.g., strings, integers).

Changes made:

Added a validation check in _process_features() to raise a ValueError when list entries are not scalars

Updated the API docstring for MetricFrame to clearly state that all sensitive_features entries must be scalar

Included a unit test test_tuple_entries under test_metricframe_process_features.py to verify the new validation

Tests

  • [ ] no new tests required
  • [x] new tests added
  • [ ] existing tests adjusted

Documentation

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

Screenshots

Mariam-ke avatar Apr 30 '25 10:04 Mariam-ke

thanks @Mariam-ke! pinging @romanlutz on this one for an opinion

TamaraAtanasoska avatar May 05 '25 10:05 TamaraAtanasoska

Float is scalar. But will it work? I think there was an issue for that at some point.

romanlutz avatar May 05 '25 15:05 romanlutz

Sorry, didn't see this before.... I'm not seeing the change to the test file?

And we did want to avoid floats as sensitive features. We really want them to be integers or strings (or enums, to the degree which Python supports such things).

riedgar-ms avatar May 07 '25 16:05 riedgar-ms

Hey @riedgar-ms all done, added the test and also updated the validation logic to explicitly disallow floats as sensitive_features

Mariam-ke avatar May 07 '25 17:05 Mariam-ke

Hi @Mariam-ke, now that CI is free of the scipy issues, it seems there is a failing test related to your PR

FAILED test/unit/metrics/test_metricframe_process_features.py::TestTwoFeatures::test_float_entries - TypeError: __init__() takes 1 positional argument but 4 positional arguments (and 1 keyword-only argument) were given

You should be able to see this if you pull from this PR and rerun the tests.

TamaraAtanasoska avatar Jul 14 '25 14:07 TamaraAtanasoska

Hi @Mariam-ke, it would be nice to get this merged! Would you be able to have a look at the small comment by @taharallouche as well as @TamaraAtanasoska's comment re. failing test?

@riedgar-ms are you happy with the other changes?

hildeweerts avatar Aug 12 '25 14:08 hildeweerts

hi @Mariam-ke, I am here if you need any help with fixing the test and getting this to a mergeable form, feel free to ping on Discord. I am not sure if all comments from @taharallouche were addressed, I believe we had an exchange above (it seem like it?)

TamaraAtanasoska avatar Sep 12 '25 11:09 TamaraAtanasoska

Hey @TamaraAtanasoska all done. please let me know if you need me to do anything else.

Mariam-ke avatar Sep 12 '25 14:09 Mariam-ke

I would say that this deserves a changelog entry!? As a user will now get an error where there wasn't before.

TamaraAtanasoska avatar Sep 15 '25 11:09 TamaraAtanasoska