Skip to content
This repository has been archived by the owner on Dec 18, 2023. It is now read-only.

Add diagnostic visualization tools #1631

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ndmlny-qs
Copy link
Contributor

This commit adds the feature to run diagnostic tools using ArviZ and
Bokeh in a Jupyter environment.

  • Added a tools sub-package in the diagnostics package. This new
    sub-package adds the following files, each for a specific tool that
    runs ArviZ model diagnostics.

    • autocorrelation
    • effective_sample_size
    • marginal1d
    • marginal2d
    • trace
  • The listed tools above also have two corresponding files, one for
    types and the other for methods used in the tool.

Resolves #1490

Motivation

Please describe your motivation for the changes. Provide link to any related issues.

Changes proposed

Outline the proposed changes and alternatives considered.

Test Plan

Please provide clear instructions on how the changes were verified. Attach screenshots if applicable.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • The title of my pull request is a short description of the requested changes.

This commit adds the feature to run diagnostic tools using ArviZ and
Bokeh in a Jupyter environment.

- Added a `tools` sub-package in the `diagnostics` package. This new
  sub-package adds the following files, each for a specific tool that
  runs ArviZ model diagnostics.

  - autocorrelation
  - effective_sample_size
  - marginal1d
  - marginal2d
  - trace

- The listed tools above also have two corresponding files, one for
  types and the other for methods used in the tool.

Resolves #1490
@ndmlny-qs ndmlny-qs added the enhancement New feature or request label Aug 29, 2022
@ndmlny-qs ndmlny-qs self-assigned this Aug 29, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 29, 2022
Copy link
Contributor

@horizon-blue horizon-blue left a comment

Choose a reason for hiding this comment

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

Thanks for working on this awesome widget and great job on adding the extensive typing & docstrings. Let me try import it to see if we can get the plotting util to work internally :)

from bokeh.plotting.figure import Figure


Figure_names = None | list[str]
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like for Python 3.8, even if you use from __future__ import annotations, it only let you use list as typing in annotations by skipping their evaluation. So we might have to use typing.List here (ditto to the rest of the doc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've reverted back to the typing.* methods. The linting error was not informative about this

@facebook-github-bot
Copy link
Contributor

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ndmlny-qs has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ndmlny-qs
Copy link
Contributor Author

hmm, linting is still failing, and I believe it is because I forgot to add the copyright notice. Fixing now.

@facebook-github-bot
Copy link
Contributor

@ndmlny-qs has updated the pull request. You must reimport the pull request before landing.

@@ -3,6 +3,7 @@
# This source code is licensed under the MIT license found in the
# LICENSE file in the root directory of this source tree.

from beanmachine.ppl.diagnostics.tools import viz
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to add this to __all__ so that the linter won't complain that this is imported but not use :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I need to setup pre-commit to point to setup.cfg as that's where the flake8 config exists. Do you know of a tool that will add copyright notices for Python?

@facebook-github-bot
Copy link
Contributor

@ndmlny-qs has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ndmlny-qs has updated the pull request. You must reimport the pull request before landing.

@ndmlny-qs
Copy link
Contributor Author

Found a bug that cropped up due to the type changes. It has been fixed in the latest push.

@facebook-github-bot
Copy link
Contributor

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ndmlny-qs has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ndmlny-qs has updated the pull request. You must reimport the pull request before landing.

@ndmlny-qs
Copy link
Contributor Author

Thanks @CactusWin for the gpytorch fix. I was struggling with errors from it earlier today.

@facebook-github-bot
Copy link
Contributor

@ndmlny-qs has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ndmlny-qs ndmlny-qs mentioned this pull request Sep 21, 2022
14 tasks
facebook-github-bot pushed a commit that referenced this pull request Oct 15, 2022
Summary:
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 `tool` folder in the `diagnostics` folder contains the proposed changes. In this folder there is a `js` folder 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.

Pull Request resolved: #1691

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

- [ ] Docs change / refactoring / dependency upgrade
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

### Checklist

- [x] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I have read the **[CONTRIBUTING](https://github.com/facebookresearch/beanmachine/blob/main/CONTRIBUTING.md)** document.
- [ ] I have added tests to cover my changes.
- [ ] All new and existing tests passed.
- [x] The title of my pull request is a short description of the requested changes.

### TODO

- [ ] Python unit tests
- [ ] JavaScript unit tests
- [ ] Figure out if the build should run `npm run build` for the tools, or if we should just have the minified code for the JS callbacks in the code base.

Reviewed By: feynmanliang

Differential Revision: D39714194

Pulled By: horizon-blue

fbshipit-source-id: 4d87a9fb4108c093f327a94ea33d604dcda68dc8
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] visual diagnostic tools
3 participants