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 option to select which model to use for counterfactual computation #2255

Merged
merged 4 commits into from
Jun 3, 2019

Conversation

tolga-b
Copy link
Contributor

@tolga-b tolga-b commented May 21, 2019

  • Motivation for features / changes
    This PR adds the ability to choose which model to use for for counterfactual example computation (Previously, it was computed with respect to first model).

  • Technical description of changes
    Added a drop-down menu to choose model name in counterfactual settings row. This panel is hidden when there is only one loaded model.

  • Screenshots of UI changes

Screenshot from 2019-05-21 09-47-57

Screenshot from 2019-05-21 09-48-18

  • Detailed steps to verify changes work correctly (as executed by you)
    I ran all demos locally and verified that counterfactuals are computed correctly for the chosen model.

  • Alternate designs / implementations considered

@tolga-b
Copy link
Contributor Author

tolga-b commented May 21, 2019

@jameswex please review.

@@ -2868,6 +2898,10 @@ <h2>Show similarity to selected datapoint</h2>
return stats && Object.keys(stats).length > 0;
},

shouldHideCounterfactualModelSelector_: function(modelNames) {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about being dependent on "numModels" instead? Then just need to check if that num != 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make sure that parsedModelNames is populated before creating the window element, in case we end up in a state where numModels is set but parsedModelNames is still being processed. Right now it seems like parsedModelNames is updated before numModels, so again it may cause some inconsistencies (e.g. numModels=1 but parsedModelNames,length=2).

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.

only one comment

@jameswex jameswex requested a review from stephanwlee May 21, 2019 18:19
// TODO (tolgab) Update for model comparison
nearestCounterfactualModelChanged_: function() {
if (this.showNearestCounterfactual) {
this.findClosestCounterfactual_();
Copy link
Contributor

Choose a reason for hiding this comment

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

I see a bit of repetition with nearestCounterfactualDistChanged_. Would it make sense to just use an observer and do

observers: [
  ...,
  'maybeFindClosestCounterFactual_(showNearestCounterfactual, nearestCounterfactualModel, nearestCounterfactualDist)'
],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done!

// Choose which model is used for counterfactual computation
nearestCounterfactualModel: {
type: Number,
value: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this number referring to an id of a model or an index? Perhaps a change to the prop name to better depict its nature might be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@jameswex jameswex merged commit efd8a94 into tensorflow:master Jun 3, 2019
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.

3 participants