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

Add ability to set custom distance function for counterfactuals #2607

Merged
merged 44 commits into from
Sep 19, 2019

Conversation

tolga-b
Copy link
Contributor

@tolga-b tolga-b commented Aug 29, 2019

  • Motivation for features / changes
    WIT did not support counterfactual computation for image and text features as it is not straightforward to define the similarity of two images or two sentences. This PR adds the ability to pass a custom python function for similarity computation such that WIT can query that function to compute counterfactuals. This allows the flexibility for users to define and experiment with their custom similary metrics and opens the option to load even a separate deep learning model for similarity purposes.
    In addition, smile detection demo is updated to use tfjs MobileNet model embeddings for example distances, allowing counterfactuals using image features (previously this demo only used tabular data for counterfactuals while smile detection model only used the image.)

  • Technical description of changes
    WitConfig now has set_custom_distance_fn to pass a python function.
    If a custom distance function is not set, WIT behaves the same as before (L1/L2 distance).
    Boilerplate code to request distances from js side and call a js callback function from the python side when the distances are ready.

  • Screenshots of UI changes
    If custom distance function is set, L1 L2 distance selection is greyed out and a third option "User-specified" appears. The same is true for show similarity to selected datapoint option.
    Counterfactuals:
    cf_mobilenet
    Show similarity to selected datapoint pane:
    pane_cf
    Facets scatterplot w.r.t MobileNet embedding distance:
    custom_distance

  • Detailed steps to verify changes work correctly (as executed by you)
    Verified jupyter notebook and colab notebook usage by creating a Smile detection demo with custom distance method that uses the difference in average image color for distance between two images.
    Ran all browser demos, including the new smile detection demo that uses custom distance function.

  • Alternate designs / implementations considered

@tolga-b
Copy link
Contributor Author

tolga-b commented Aug 29, 2019

@jameswex I will do one more pass on this to check if everything works correctly (demos etc.). addDistance with custom distance function will have it's own PR. You can start checking it now or after that pass, thank you!

@mpushkarna
Copy link
Contributor

@tolga-b cc:@jameswex
For a complete UI implementation, when users set a custom distance function:
(a) If users can only use the custom distance metric, radio buttons for L1 or L2 distances should be disabled or
(b) Users should be able to pick from L1, L2 and custom distance in a dropdown.

Copy link
Contributor

@jameswex jameswex left a comment

Choose a reason for hiding this comment

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

how much work would it be to add this for adding distance feature as well? want in separate PR?

@jameswex
Copy link
Contributor

@tolga-b cc:@jameswex
For a complete UI implementation, when users set a custom distance function:
(a) If users can only use the custom distance metric, radio buttons for L1 or L2 distances should be disabled or
(b) Users should be able to pick from L1, L2 and custom distance in a dropdown.

I think if users set their own custom distance measure that we can always use that and remove the controls for l1 and l2, as in these cases the auto-computed measure doesn't make sense (like we currently see in our text and image examples).

@jameswex
Copy link
Contributor

jameswex commented Sep 4, 2019

what is left to get this fully working with appropriate UI? if nothing, then can you also update the smiling demo and a notebook demo with custom distance so it can be tested in this PR? Thanks.

@wchargin
Copy link
Contributor

(I cancelled this build because the first two jobs have lint errors, so
the rest would have failed in the same way, and I’d like to unblock CI
for #2651, which blocks a 1.15 release. Hope you don’t mind.)

@jameswex
Copy link
Contributor

@stephanwlee I have reviewed this code. would you or another TB team member also like to before it is merged?

third_party/js.bzl Outdated Show resolved Hide resolved
this.inferMutantsCounter = 0;
this.view_.addEventListener('infer-mutants', (e) => {
e.detail['infer_mutants_counter'] = this.inferMutantsCounter++;
this.model.set('infer_mutants', e.detail);
this.mutantFeature = e.detail.feature_name;
this.touch();
});
this.computeDistanceCounter = 0;
this.view_.addEventListener('compute-custom-distance', (e) => {
e.detail['compute_distance_counter'] = this.computeDistanceCounter++;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

third_party/js.bzl Outdated Show resolved Hide resolved
callback: callbackFun,
params: params,
};
this.fire('compute-custom-distance', urlParams);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@tolga-b
Copy link
Contributor Author

tolga-b commented Sep 18, 2019

Thank you, @stephanwlee I addressed the review comments. Could you please check it out?

third_party/js.bzl Show resolved Hide resolved
this.inferMutantsCounter = 0;
this.view_.addEventListener('infer-mutants', (e) => {
e.detail['infer_mutants_counter'] = this.inferMutantsCounter++;
this.model.set('infer_mutants', e.detail);
this.mutantFeature = e.detail.feature_name;
this.touch();
});
this.computeDistanceCounter = 0;
this.view_.addEventListener('compute-custom-distance', (e) => {
e.detail['compute_distance_counter'] = this.computeDistanceCounter++;
Copy link
Contributor

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?

@tolga-b
Copy link
Contributor Author

tolga-b commented Sep 18, 2019

Thank you Stephan, I removed the side effect in wit.js event object per your suggestion!

@jameswex jameswex merged commit 15456be into tensorflow:master Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants