DOC Validate and document scalar entry requirement for sensitive_features
…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
thanks @Mariam-ke! pinging @romanlutz on this one for an opinion
Float is scalar. But will it work? I think there was an issue for that at some point.
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).
Hey @riedgar-ms all done, added the test and also updated the validation logic to explicitly disallow floats as sensitive_features
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.
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?
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?)
Hey @TamaraAtanasoska all done. please let me know if you need me to do anything else.
I would say that this deserves a changelog entry!? As a user will now get an error where there wasn't before.