Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add ability to set custom distance function for counterfactuals #2607
Add ability to set custom distance function for counterfactuals #2607
Changes from 10 commits
ae2daff
d6a6920
0359655
fc6709b
31e05ae
79dd89b
00cade8
ed3a4c6
299dd47
76ddff8
0ee315a
a5126e8
9749fd6
1343f30
cc2ab68
3fb28c1
47b5960
243e78e
4eb6e09
9e98468
674afd4
4faa9e6
a024f6d
4a9e9e8
16039ef
06a8451
eadecfa
3bccfc8
d2414d9
0c4a9c9
2469392
859cfa6
87eb749
d38f7b5
50839a3
7c82372
9aa1a72
8b02c04
de0ca1b
69f5d6d
b87964c
5becf6a
a80af0f
13a8444
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it weird to see notebook specific code in the main dashboard code. What is the expected behavior outside of the notebook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requestDistanceWithCallback would not be invoked in non-local instances (local=demos and notebook). In case we are in non-local mode, WIT defaults to it's previous behavior of computing counterfactuals completely on the js side with L1 and L2 distance between examples. This is slightly similar in terms of behavior to custom_predict_fn where it is only supported in notebook mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added TODO note here so that if there is a support in TensorBoard mode to provide custom distance functions for counterfactuals then we should update this function to reflect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels quite odd and not clean. Would it be possible to use data binding instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The counter is there as it is in the case of infer-mutants above. We use data binding for what is passed ('compute_custom_distance' is set to e.detail). However, we only pass the example id back to the python side instead of the full example since examples are already in sync between the backend and frontend. In this case, if someone requests the same example again after updating some features, the change on bound dict may not trigger since it is the same dict. We force trigger it by incrementing the counter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but I must confess I am very lost. Even if this logic is correct, I do think this has readability problem.
AFAICT, the event is fired from JavaScript side. When firing the event, the object does not contain the property
compute_distance_counter
. 1. how is it being used by owner of the event object? 2. where does the Python come into play?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dashboard launches this event from js side by passing some parameters (example index, callback ind etc.). "compute_custom_distance" is sync variable between js and python sides. This synchronization is managed by jupyter widget framework. When we call this.model.set('compute_custom_distance', e.detail), ideally jupyter should recognize the change in "compute_custom_distance" and trigger distance computation. James and I found out previously (for infer-mutants) that if someone edits an example then fires this event again from the dashboard, since e.detail does not contain the contents of the example but only it's index, jupyter thinks nothing has changed and model.set does not trigger distance computation. James solved it by adding a counter so that we make sure the serialized dictionary always has one field that changes between calls and jupyter triggers the sync.
Since this is a jupyter specific issue, we did not include this in the event but append it in wit.js related to jupyter.