Marginal diagnostic tool #1691
Marginal diagnostic tool #1691
Conversation
This commit includes the marginal 1D diagnostic tool with JavaScript callbacks.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
facebook-github-bot
added
the
CLA Signed
|
@horizon-blue you will need to run |
|
@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
- Removes ArviZ as a dependency from the diagnostic tool. - Adds jsdoc Documentation for the TypeScript methods. - Fixes flake8 linting error. - Adds clarifying comments to the tool. - Adds missing copyright notice.
|
@ndmlny-qs has updated the pull request. You must reimport the pull request before landing. |
|
@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
The task of getting Node to work with our internal Buck ends up taking a bit longer than I initially expected. I think I just found a way to install node modules with our internal tools, but haven't got the time to try getting webpack to work or get compiled JS into the Python library (will probably try those out tomorrow) :). In the meantime, could you help updating three items here?
|
|
@ndmlny-qs has updated the pull request. You must reimport the pull request before landing. |
|
@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
- Aligns type names between Python and TypeScript. There is now a one-to-one correspondence between Python TypedDict objects and the TypeScript interface objects. - Redundancy in the Python TypedDict objects has been removed. - Fixes tooltips not rendering correctly. - Updates the link in the Help Tab of the tool such that when clicked it opens a new tab in the browser. - Moves a few peerDependencies to the devDependencies section in the package.json file. This was done because yarn issued a warning indicating there were missing peer dependencies. - Refactors the tool such that no calculations for the data displayed is done in Python. All updates and all calculations for the tool data is done exclusively in the browser. The rational for this change was to prevent users from having initial data calculated by Python, and subsequent updates calculated by JavaScript in the browser when interacting with the tool. There are notes in the code indicating why this change was made.
|
@ndmlny-qs has updated the pull request. You must reimport the pull request before landing. |
|
@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Okay, looks like I managed to get it to work in Bento... to some extent :). For some reason no plot is rendered when the widget is first loaded and the console says
Though after selecting any of the query from the drop down menu, the plot shows up just fine.
I tried pulling your latest change to my own machine and observe the same issue with my classic Jupyter notebook, which leads me to wonder if it's a but in the code rather than a Bento issue. @ndmlny-qs do you mind giving this a look next week to see if there's a bug in your newest version of the widget? |
|
@horizon-blue let me know what I need to modify for the internal tests to green light. Thanks again, you are super spectacularly awesome! |
|
@ndmlny-qs As I posted last Friday, I think there might be a bug in your latest version of the widget that makes it fail to fully render when it first loads up. I tried running the widget on my local Jupyter notebook session and it has the similar issue, so I believe this is not caused by our internal config. Could you check and see if you can reproduce the same issue on your side? |
|
Ah yes, for some reason I did not read your post last Friday. This unfortunately is a technical hurdle that we cannot overcome due to the version of Bokeh we are using. I did investigate finding an Relevant issues and PRs from Bokeh
The reason why I actively chose to do what you see (a blank tool) on first render, is that I could not figure out a way to get Python to issue a callback on the tool without user interaction. I wanted Python to make the tool, but not to calculate any data to bind to the glyphs in the tool. This is why the console says I know this may sound like an absurd thing to do, but if I didn't do it this way then Python would calculate the initial values for the glyphs and render them to the tool with an initial view just fine. This is great, but when the browser took over running calculations due to user interactions, the hand-off between Python and JavaScript would cause a significant flicker in the rendered tool that is quite jarring. The only way around this flicker was to prevent Python from doing any of the calculations, and to make the browser do all of them. The only way to do that was to make a user interact with the tool such that all data is calculated in the Browser. I made this decision to prevent a user from having to deal with the render flickering as data is handed over from Python to the Browser. In doing so, I solved one issue but have caused a different UX issue: the user has to interact with the tool before they see any of the model statistics. I could be completely wrong about this decision, and welcome any opinions you may have. |
There was a problem hiding this comment.
I see, thanks for the explanation! I do believe would be more intuitive if we can render something by default, but the current way isn't too bad either, as long as we make it clear to the user that they actually need to interact with the widget so they wouldn't mistaken this as a bug (just like I did). One thing we can also consider as a follow up is to add a text or something in the widget that hints the users to click the drop down menu, till this issue gets resolved in Bokeh 3.0.
Since this pull request has been pending for a while, I'm just going to try to get it merged internally as-is to make it easier to work on follow up changes. I'm also going to revert the changes in the coin flipping tutorial until we are able to render the widget in docusaurus.
|
@horizon-blue That's a great idea, and I will propose an update with a more intuitive (click-me) interaction for the user in this PR. I am currently refactoring and updating the notebook converter in a different branch that will embed Bokeh Applications in Docusaurus. My initial experiments showed it is possible, and I am working to automate it. |
|
@horizon-blue let me know what you think of this design. For some reason the mouse isn't showing, but you can get the idea of the gif.
|
- Moved all Bokeh embed from the tool class to the base class. - Updated tool class docs. - New method on the base class that contains an HTML template for more intuitive required UI for better UX. - Reordered how the tool view is created in order to attach the new CSS class for the UI/UX updates.
|
@ndmlny-qs has updated the pull request. You must reimport the pull request before landing. |
|
@ndmlny-qs has updated the pull request. You must reimport the pull request before landing. |
|
@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| # This source code is licensed under the MIT license found in the | ||
| # LICENSE file in the root directory of this source tree. | ||
|
|
||
| """Accessor definition for extending Bean Machine `MonteCarloSamples` objects.""" |
There was a problem hiding this comment.
Can you help me understand what is the purpose of this?
There was a problem hiding this comment.
I've updated the docstrings to include an explicit example for what this is doing.
| from beanmachine.ppl.inference.monte_carlo_samples import MonteCarloSamples | ||
|
|
||
|
|
||
| def serialize_bm(samples: MonteCarloSamples) -> Dict[str, List[List[float]]]: |
There was a problem hiding this comment.
again, possible to use InferenceData instead of MCS?
There was a problem hiding this comment.
It is, however, I think we should leave this as a future feature request since these tools are pretty feature loaded as is right now.
- Refactors dosctrings from NumPy to Google style. - Added a prettierrc for dev and updated the package.json, yarn.lock files - Refactored the marginal1d tool for responsive size changes and updated to the format that allows Docusaurus integration. - Added more documentation around the accessor objects with an explicit example in the docstring.
|
@ndmlny-qs has updated the pull request. You must reimport the pull request before landing. |
|
@ndmlny-qs has updated the pull request. You must reimport the pull request before landing. |
|
@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This commit includes the marginal 1D diagnostic tool with JavaScript callbacks.
Motivation
This PR completes one tool that uses Bokeh and JavaScript callbacks in order to create an interactive tool that can be viewed in Jupyter. This refactors the code in PR #1631 heavily, since pure Python callbacks were found to not function properly with internal tools.
Changes proposed
A new
toolfolder in thediagnosticsfolder contains the proposed changes. In this folder there is ajsfolder that contains all the JavaScript callbacks needed for the Bokeh tool. The tool creates plots of marginal distributions for each random variable of the model. The output is a self-contained HTML object that can be rendered in Jupyter without any external CDN calls for JS resources.Test Plan
Unit tests for the Python and JavaScript will be done at a later commit. Right now the testing was to run the tool in the Coin Flipping tutorial, and to inspect the output and ensure only static resources were used.
Types of changes
Checklist
TODO
npm run buildfor the tools, or if we should just have the minified code for the JS callbacks in the code base.