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

fix failing vision tests due to new RAI validation logic which doesn't allow non string dropped column names #2302

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

imatiach-msft
Copy link
Contributor

Description

After responsibleai v0.30.0 package release, one of the vision tests started failing due to new validation logic introduced in the v0.30.0 package:

        for column_name in column_names:
            if not isinstance(column_name, str):
>               raise UserConfigValidationException(
                    'Entries in {} must be strings.'.format(column_purpose))
E               raiutils.exceptions.UserConfigValidationException: Entries in dropped feature must be strings.

The new validation logic does not allow dropped features to be non-strings (in this case integers). However, pandas does allow integer column names and in this case the dataset in the test had integer column names. To fix the test, I modified the pandas dataframe integer columns to be text type instead of int type, so that the new validation logic would pass. An alternative fix might be to allow dropped features to have integer values instead of strings.

In addition I changed iloc to use two dimensional indexing always since I noticed the behavior could be different than expected when using metadata features with iloc indexing followed by row index if index is not reset as a safeguard - which I noticed while debugging.

Checklist

  • I have added screenshots above for all UI changes.
  • I have added e2e tests for all UI changes.
  • Documentation was updated if it was needed.

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2023

Codecov Report

Merging #2302 (4bdd8a3) into main (e266587) will decrease coverage by 9.74%.
Report is 2 commits behind head on main.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #2302      +/-   ##
==========================================
- Coverage   88.86%   79.12%   -9.74%     
==========================================
  Files         133       26     -107     
  Lines        7550     2199    -5351     
==========================================
- Hits         6709     1740    -4969     
+ Misses        841      459     -382     
Flag Coverage Δ
unittests 79.12% <66.66%> (-9.74%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...n/responsibleai_vision/utils/feature_extractors.py 93.10% <50.00%> (ø)
...ext/responsibleai_text/utils/feature_extractors.py 94.55% <100.00%> (ø)

... and 108 files with indirect coverage changes

@imatiach-msft imatiach-msft merged commit 225bbf1 into main Aug 31, 2023
@imatiach-msft imatiach-msft deleted the ilmat/fix-vision-tests branch August 31, 2023 14:13
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.

3 participants