flatironinstitute / stan-playground

Run Stan models in the browser
Apache License 2.0
36 stars 1 forks source link

Add data generation feature with Python and R support #127

Closed magland closed 4 months ago

magland commented 4 months ago

Adds support for data generation with Python or R

As we discussed, there is a new data generation tab on the right hand view. Then user can select Python or R language. I copied over code from #94 for the data generation editors.

I needed to put this on top of #120 because it uses the pyodide worker.

We had discussed leaving the data.json in an edited state after generation, but I now don't see the value in it because there's no way to revert to previous state anyway.

jsoules commented 4 months ago

My instinct says there's a lot of duplicated code between the DataPyFileEditor.tsx and DataRFileEditor.tsx, as well as with the DataGenerationWindow.tsx and other editors more broadly. I don't know that we need to hold this up pending a full unification/simplification with the other editors, but I'd like to discuss when you're back some of the differences between the R and Py versions of that editor at least.

magland commented 4 months ago

Merged bmw/analysis-py into this branch, and updated the data.py data.r handling to use the new interface.

Everything went smoothly, but one thing to note:

Added loadDraws: boolean as part of the settings for the worker. This is in addition to showPlots and producesData. I think this makes more sense than looking at whether spData exists, since I was thinking we might want to use spData for other things in the future aside from draws. This simplified getScriptParts() because we no longer need to pass spData into it, just for the check.

magland commented 4 months ago

@jsoules ... just noticed your comment about duplicated code. I'll have a look. (still on vacation, but have time to work on this here and there)

magland commented 4 months ago

@jsoules I took a look at the duplicated code between DataPyFileEditor and DataRFileEditor and it seemed like the toolbarItems warranted a shared function so I separated that out. I think it will make sense to make a global toolbarItems definition across all the editors, but I suggest we do that in a separate PR. Aside from that, I feel like these two editor components now have sufficiently different functionality as to keep them separate. Perhaps it does make sense to make a common interface for all the editor components, but again would suggest that be done in a separate issue.

magland commented 4 months ago

@WardBrian I incorporated your usePyodideWorker hook into this branch. Now you can implement the singleton approach you were proposing.

(Do we want to merge or continue keeping this PR separate?)

WardBrian commented 4 months ago

(Do we want to merge or continue keeping this PR separate?)

I can go either way -- there are parts of this that are probably easier to review independently, but also a lot of the analysis PR is currently speculating about the eventual implementation of the data, so perhaps it makes sense to do monolithically.

@jsoules?

magland commented 4 months ago

Merged changes from main (especially CSS styling) and resolved a few minor conflicts.

Added a console message "Data updated" that appears after the data generation runs and the data.json has been updated.

WardBrian commented 4 months ago

Merging into #120 so review can continue there