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

Consolidate script editors, add analysis.R #157

Merged
merged 16 commits into from
Jul 26, 2024
Merged

Consolidate script editors, add analysis.R #157

merged 16 commits into from
Jul 26, 2024

Conversation

WardBrian
Copy link
Collaborator

I decided to open this as a replacement to PR #155 rather than just pushing to that branch because I did a bunch of code moves.

This PR:

  • Adds two new components, ScriptEditor and its big brother, PlottingScriptEditor. These are the basis of the data and analysis tabs.
  • Refactored the existing tabs to share as much code as is reasonable, and use those components
  • Reorganize the folder structure of this code (everything that touches webr/pyodide now entirely lives under app/Scripting) and move any large R/Python code chunks out of strings and into their own dedicate files.
  • Add the analysis.R window, with the loading code for the draws based on posterior.

Only this final bullet point changes any of the functionality, the rest is just tech debt cleanup.

Closes #106
Closes #58

@magland
Copy link
Collaborator

magland commented Jul 26, 2024

Very Cool!

Before I start reading the code, I wanted to test to see if everything is working. One thing I noticed (I think it was introduced in this PR). When I run the python data generation script it always needs to reload pyodide. Is the worker getting terminated?

data.R does not have this problem.

analysis.py and analysis.R also do not have this problem.

@WardBrian
Copy link
Collaborator Author

WardBrian commented Jul 26, 2024

@magland I observe the behavior you are describing on the current main at https://stan-playground.flatironinstitute.org/, so I don't think it originates here. I have a hunch about where it would come from, let me see if I'm correct.

Otherwise, another point in favor of #142, possibly

@WardBrian
Copy link
Collaborator Author

@magland I believe it is fixed. Basically, the onData part of the callback was changing after every run, since it depended on the data, so it re-loaded the usePyodide hook.

Copy link
Collaborator

@magland magland left a comment

Choose a reason for hiding this comment

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

Just a few minor comments. We should merge this soon because there are so many changes and renames.

@WardBrian
Copy link
Collaborator Author

@magland done, and I agree I'd prefer to merge this before too much else just due to conflicts. I'd still appreciate if @jsoules could take a look, since some of the consolidation was a request of his

Copy link
Collaborator

@jsoules jsoules 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 looks fine. There's a couple minor things that might be slightly cleaner, but I'd also be happy to just merge as is.

gui/src/app/Scripting/Analysis/AnalysisPyWindow.tsx Outdated Show resolved Hide resolved
gui/src/app/Scripting/Analysis/useAnalysisState.ts Outdated Show resolved Hide resolved
@WardBrian WardBrian merged commit 5560953 into main Jul 26, 2024
2 checks passed
@WardBrian WardBrian deleted the bmw/analysis.R branch July 26, 2024 19:17
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.

Attempt to unify editor components Integrate webr and Pyodide to allow analysis of the samples
3 participants