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

frontend: sampling output view (take 3) #47

Merged
merged 7 commits into from
Jun 14, 2024
Merged

Conversation

magland
Copy link
Collaborator

@magland magland commented Jun 7, 2024

This is a redo of #46

The difference is:

However, it does still have Mean, StdDev and the percentiles.

I verified that these are correct by downloading to a spreadsheet and calculating these there.

@WardBrian I know that you tried this using stansummary here
#32 (comment)

... and there was discrepancy even for the mean. Could it be that stansummary is doing something other than computing the mean of all the draws? For the stddev and percentiles I can understand that it makes a difference whether it averages over the chains or treats it as one big pool. But for mean, the average of averages is the same as the average of all (for equal number of draws).

So can we check whether stansummary is doing something other than computing the means of all the draws?

I feel confident that these columns are being correctly computed in SP (assuming simply mean over all draws, etc). You can double-check that when you compare with stansummary.

I think we should settle this discrepancy in this PR. Then a separate PR for the more complicated ess-related ones.

@magland
Copy link
Collaborator Author

magland commented Jun 7, 2024

Is there a way to install stansummary without building cmdstan? I installed cmdstan with conda (conda-forge) but I didn't get stansummary or cmdstan_summary

@WardBrian
Copy link
Collaborator

stansummary will be part of the built conda package, but it isn't added to your $PATH by default

It lives at $CMDSTAN/bin/stansummary

@magland
Copy link
Collaborator Author

magland commented Jun 7, 2024

Thanks. I tried it with stansummary (separated it out into 4 .csv files as you did) and I get results that are consistent with SP for mean, stdev, and percentiles. The others (ess, ....) were not exactly equal.

This was with the default linear model example we've been using and seed=1

@magland magland requested review from jsoules and WardBrian June 7, 2024 16:26
Copy link
Collaborator

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

A few questions before the weekend -- thanks for splitting this out again.

gui/src/app/SamplerOutputView/SequenceHistogramWidget.tsx Outdated Show resolved Hide resolved
gui/src/app/StanSampler/StanSampler.ts Show resolved Hide resolved
@@ -21,11 +21,14 @@
"@mui/icons-material": "^5.15.17",
"@mui/material": "^5.15.17",
"monaco-editor": "^0.48.0",
"plotly.js": "^2.33.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

[note] it may be worth using an alternative plotly distribution for size if we only need certain kinds of plots. For stan-web-demo this made a pretty significant difference in overall bundle size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing that out, I didn't know it was available.

Not an easy change right now since I am using react-plotly (npm package) which seems to only work with the full plotly.js. But I'm sure there's a workaround.

I'll note that since we are doing lazy loading, the plotly.js doesn't contribute to the main bundle on initial page load.

Shall we defer for a separate issue when we can figure out how to work around limitations of react-plotly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, we can open a ticket to try it later. I know react-plotly supports it, but I’m not sure how this interacts with the lazy loading, and as you say the lazy loading helps make it less necessary

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For information, I just tried using a smaller distribution and ran into some bundling problems, so yeah I think we should do this in a different ticket.

@WardBrian WardBrian linked an issue Jun 13, 2024 that may be closed by this pull request
@WardBrian WardBrian mentioned this pull request Jun 13, 2024
2 tasks
Copy link
Collaborator

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

I think this is in a good place. I had one comment on the typing in the LazyPlot component.

Also, I don't think this is a new behavior here, but just noting that some really odd rendering happens if you drag the separator of the output area to the top of the window:

image

gui/src/app/components/LazyPlotlyPlot.tsx Outdated Show resolved Hide resolved
@magland magland merged commit 2c0e884 into main Jun 14, 2024
1 check passed
@WardBrian WardBrian deleted the sampling-output-take-3 branch June 14, 2024 21:08
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.

Sample output views: Plots
2 participants