-
Notifications
You must be signed in to change notification settings - Fork 313
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
InteractionPlot refactor #3097
base: main
Are you sure you want to change the base?
InteractionPlot refactor #3097
Conversation
Differential Revision: D65234219
Differential Revision: D65233907
Summary: As titled. Also renames InteractionAnalysis --> InteractionPlot to conform to convetions in this ax.analysis.plotly module. This is the second in a series of diffs that will get this Analysis ready for Ax 1.0, including: * Removal of dependency on ax.plot * Easier options in `InteractionPlot.__init__` * Misc tidying, pyre fixmes, etc Differential Revision: D65089145
Summary: Simplifying InteractionPlot by removing some arguments from its initializer. If this simplifications seem appropriate then I will continue to simplify the Plot. Removals: * top_k: Prefer to show sobol indices for all components on bar chart, slice/sufrace for top 6 always * data: Let's always use the data on the experiment * most_important: Always sort most important to least important, never least to most * display_components: Always display components * decompose_components: Never decompose components * plots_share_range: Always share range * num_mc_samples: Always use 10k samples * [RFC] model_fit_seed: Do not bother with seed setting -- we dont do this for any other plots so its probably not worth the complexity here The following diffs will restructure the code here to take advantage of the simplified options Differential Revision: D65148289
This pull request was exported from Phabricator. Differential Revision: D65234856 |
Summary: Pull Request resolved: facebook#3097 When this is landed we will be able to use this plot in Ax 1.0 and Ax UI. Refactor the interaction plot to be in line with our structure for ax.analysis. This includes a massive reduction in overall code (about half) and a full decoupling from ax.plot. Adds robustness features around generating subplots -- a failed surface subplot will no longer fail the full analysis. This new version of the plot is slightly more opinionated in that we always plot both the feature importance bar chart AND the top 6 features, always plots top 15 components in the bar chart, never decomposes components, and always has plots share scale. These settings are most useful and help drastically simplify the code, so I think we should keep them for now and only consider adding them back if there is demand. Differential Revision: D65234856
This pull request was exported from Phabricator. Differential Revision: D65234856 |
36cac30
to
4e95ad2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3097 +/- ##
==========================================
- Coverage 95.74% 95.73% -0.01%
==========================================
Files 488 496 +8
Lines 49769 50060 +291
==========================================
+ Hits 47653 47927 +274
- Misses 2116 2133 +17 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Summary:
When this is landed we will be able to use this plot in Ax 1.0 and Ax UI.
Refactor the interaction plot to be in line with our structure for ax.analysis. This includes a massive reduction in overall code (about half) and a full decoupling from ax.plot.
Adds robustness features around generating subplots -- a failed surface subplot will no longer fail the full analysis.
This new version of the plot is slightly more opinionated in that we always plot both the feature importance bar chart AND the top 6 features, always plots top 15 components in the bar chart, never decomposes components, and always has plots share scale. These settings are most useful and help drastically simplify the code, so I think we should keep them for now and only consider adding them back if there is demand.
Differential Revision: D65234856