Remove zeros from interaction lookup, check in test#424
Remove zeros from interaction lookup, check in test#424mmschlk merged 5 commits intommschlk:mainfrom
Conversation
|
Hi @CloseChoice, welcome to shapiq! Happy to have you here! :) |
| assert (shapley_interactions[()] - emptyset_prediction) ** 2 < 10e-7 | ||
| for v in moebius_converter._computed.values(): | ||
| # check that no 0's are in the interaction lookup (except for the empty set, which is the first entry) | ||
| interaction_lookup = v.interaction_lookup |
There was a problem hiding this comment.
This part here should be wrong and we should check in the InteractionValues.interactions dictionary, which maps the interactions onto the scores. After our latest refactor of the InteractionValues and Game objects, we kind-of removed the lookup and values attributes. Originally we split the data structure into two attributes values a vector containing the interaction scores and interaction_lookup mapping interactions (tuples) to the index of the score in the values. Now we combined both things into one dictionary (interactions) which contains the scores.
TLDR: So basically we need to check weather values in interactions contain zero and not the lookup mapping.
There was a problem hiding this comment.
@mmschlk thanks for the review and the comment. I updated this to only check the interaction values and the corresponding interactions dict. Then I discovered some flaky tests, and found the rng in SOUM that reproduced the failures. I realized that we don't include interaction values that are 0, but we don't get of those that sum to zero (e.g. +0.1 and -0.1). I modified the code for this. The code is quite repetitive and could be refactored into a utils function, but I leave that to a further PR, don't think doing too much in one is helpful.
Happy for another review of yours!
CloseChoice
left a comment
CloseChoice
left a comment
There was a problem hiding this comment.
hi, updates are in and hopefully I understood your comment correctly! Let me know what you think
| assert (shapley_interactions[()] - emptyset_prediction) ** 2 < 10e-7 | ||
| for v in moebius_converter._computed.values(): | ||
| # check that no 0's are in the interaction lookup (except for the empty set, which is the first entry) | ||
| interaction_lookup = v.interaction_lookup |
There was a problem hiding this comment.
@mmschlk thanks for the review and the comment. I updated this to only check the interaction values and the corresponding interactions dict. Then I discovered some flaky tests, and found the rng in SOUM that reproduced the failures. I realized that we don't include interaction values that are 0, but we don't get of those that sum to zero (e.g. +0.1 and -0.1). I modified the code for this. The code is quite repetitive and could be refactored into a utils function, but I leave that to a further PR, don't think doing too much in one is helpful.
Happy for another review of yours!
mmschlk
left a comment
mmschlk
left a comment
There was a problem hiding this comment.
Nice thank you for the adjustments. This is correct. Also thank you for the additional SOUM tests! This is very nice!
2 participants
Motivation and Context
closes #369.
Prevented zeros from being written to the
interaction_lookup.I added this to the test, but it seems like only the change in here runs through the tests. If more thorough tests are desired, let me know.
Public API Changes
How Has This Been Tested?
see the changes in the tests.
Checklist
CHANGELOG.md(if relevant for users).