Skip to content

Conversation

@desilinguist
Copy link
Collaborator

@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
Member

@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

4 participants