Show the correlation stats as a table #3754
Conversation
wpotrzebowski
left a comment
wpotrzebowski
left a comment
There was a problem hiding this comment.
It works as expected, however I have some questions related to nomenclature.
It currently says Correlation Table but I think Statistics will be more appropriate here (we also have Correlation plot and it can be bit misleading in this context too).Also "best" for the parameter can probably be changed to something more accrurate. I think in Bayesian teminology it is a "point estimate". Maybe keeping "best" but adding tooltip will work.
|
Indeed, I think the table is produced by bumps so the nomenclature in the table would have to be changed there I think? we could put an issue into bumps perhaps? |
|
The column headings are setup on sasview end but indeed it would be good to be consistent with bumps |
|
The headers are based on bumps headers. |
|
Also, refactored the parsing out of plot() and used bumps' own method to get the values. Thanks, @wpotrzebowski ! |
|
I would suggest naming it "Parameter Statistics" or even "Statistics". Correlation suggests that it should be between two parameters and here I believe it is "per" parameter. |
…sasview into 2505-dream-output-table
|
These are statistics for the posterior distribution of the Bayesian inverse problem Possible names include "Statistics", "Parameter statistics", "Fit Results", "Summary Statistics". "best" is the best value seen by the MCMC walker. It is an estimate of the parameter vector with the maximum likelihood. I figured "best" was clearer label than "MLE". I also considered "mode" to go along with "mean" and "median", but that is misleading for multimodal distributions. |
|
BTW, the points are still available. If you had a way to enter expressions, a user could see results for derived parameter such as the cylinder volume. We could add a standard set of derived parameters for each model and calculate them at the end of the fit. |
krzywon
left a comment
krzywon
left a comment
There was a problem hiding this comment.
Tested locally, and the table looks good. I would suggest, if possible, we add this into the Uncertainty tab instead of its own tab since this summarizes that view anyway.
| plot_corrmatrix(draw=draw, fig=self.figure) | ||
| self.canvas.draw_idle() | ||
|
|
||
| class CorrelationTable(QtWidgets.QWidget): |
There was a problem hiding this comment.
For consistency, should this be called 'CorrelationTableView'? Also, isn't this a table of the Uncertainties, so UncertaintiesTableView would make the most sense, right?
|
@pkienzle @krzywon @wpotrzebowski - can you please decide on the proper naming for the tab and its location? It seems each person has a different view :) |
butlerpd
left a comment
butlerpd
left a comment
There was a problem hiding this comment.
I think we are starting split hairs a bit here.
From what I can see there is broad agreement to NOT use the word "correlation" which I think is clear. As @pkienzle who authored the table in bumps clearly points out, this has nothing to do with correlations.
I would personally choose "statistics" from @pkienzle choices (or even "summary statistics" but I don't see the necessity of adding summary) as that is exactly what it is and it avoids duplicating the word "uncertainty" which is already used. I am however not wedded to a particular nomenclature as long as it does not involve "correlations." Another option would be "confidence intervals" which is what many people call this but I know @pkienzle has strong negative feelings about that word.
I take Jeff's point which at its core is really about the fact that all the other tabs in that panel are plots whereas this is a table. That said I think it is fine to leave it where it is, particularly with the "statistics" suggested by @pkienzle. Placing it at the bottom of the uncertainty plots I think just makes it harder to find and still makes one tab unique in that it is more than just a plot.
Bottom line, removing the "correlation" word seems to be the only real issue IMO
|
The table caption currently reads "Statistics" |
|
I was speaking to the class name, mostly, not the tab name when I suggested |
fair enough. Having correlation in the class name could cause confusion to developers I suppose if it is a statistics table I suppose? I agree though it can be merged either with or without that small change in code nomenclature as far as I'm concerned. |
|
The Bayesian "credible interval" is different from a "confidence interval". I'm computing the credible interval so I would prefer to use the correct terminology. I'm not against confidence intervals per se, though I do have a preference for credible intervals because I understand them. |
|
Regarding the name of the widget, how general is it I wonder? for example if one wanted at some point in the future to show the Jacobian matrix would this widget work? If so leaving the name seems fine since in some sense correlations between parameters and uncertainties on same are ... well .. .correlated? If however the widget is really unique to this specific table than Jeff's suggestion would probably be helpful - or even "CredibleIntervalTable"? |
Description
Added a correlation stats table in the DREAM's Result Panel tabbed widget
Fixes #2505
How Has This Been Tested?
Local dev build tests
Resulting table:
Review Checklist:
[if using the editor, use
[x]in place of[ ]to check a box]Documentation (check at least one)
Installers
Licensing (untick if necessary)