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

Use example IDs when specifying sample_ids #613

Merged
merged 8 commits into from
Jul 11, 2023

Conversation

desilinguist
Copy link
Member

@desilinguist desilinguist commented Jul 10, 2023

  • Assume that sample_ids are now actual example IDs instead of indices.
  • Raise a ValueError if anything goes wrong in select_examples.
  • Update test_explanation_utils.py
    • Use actual string IDs in the test data.
    • Update all expected outputs to use these new string IDs.
  • Update relevant test in test_experiment_rsmexplain.py to IDs.
  • Update rsmexplain documentation
    • Update description of sample_ids and move it to the end.
    • Fix some typos.

Closes #609.

- Assume that `sample_ids` are now actual example IDs instead of indices.
- Raise a `ValueError` if anything goes wrong in `select_examples`.
- Use actual string IDs in the test data.
- Update all expected outputs to use these new string IDs.
- Update description of `sample_ids` and move it to the end.
- Fix some typos.
@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (e0a1392) 95.89% compared to head (1cee0cc) 95.90%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #613   +/-   ##
=======================================
  Coverage   95.89%   95.90%           
=======================================
  Files          59       59           
  Lines        9286     9297   +11     
=======================================
+ Hits         8905     8916   +11     
  Misses        381      381           
Impacted Files Coverage Δ
tests/test_experiment_rsmexplain.py 96.29% <ø> (ø)
rsmtool/rsmexplain.py 92.03% <100.00%> (+0.12%) ⬆️
tests/test_explanation_utils.py 98.80% <100.00%> (+0.12%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@damien2012eng damien2012eng left a comment

Choose a reason for hiding this comment

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

LGTM!

It's possible that a given data file has integers as ID. RSMTool will read them
as integers and not strings. In that case, we need to make sure that any sample
IDs specified in the configuration file are also converted to the same data type
as the featureset IDs before being compared.
@desilinguist desilinguist merged commit d372d66 into main Jul 11, 2023
@delete-merged-branch delete-merged-branch bot deleted the 609-use-example-ids-in-samples branch July 11, 2023 17:40
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.

rsmExplain sample ids would be better to take actual ids instead of sample indexes
3 participants