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

Add data generation feature with Python and R support #127

Merged
merged 23 commits into from
Jul 22, 2024
Merged

Conversation

magland
Copy link
Collaborator

@magland magland commented Jul 10, 2024

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
Copy link
Collaborator

jsoules commented Jul 15, 2024

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
Copy link
Collaborator Author

magland commented Jul 17, 2024

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
Copy link
Collaborator Author

magland commented Jul 17, 2024

@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
Copy link
Collaborator Author

magland commented Jul 17, 2024

@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
Copy link
Collaborator Author

magland commented Jul 17, 2024

@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
Copy link
Collaborator

(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
Copy link
Collaborator Author

magland commented Jul 18, 2024

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
Copy link
Collaborator

Merging into #120 so review can continue there

@WardBrian WardBrian merged commit a56fd25 into analysis-py Jul 22, 2024
@WardBrian WardBrian deleted the data.py.2 branch July 23, 2024 19:31
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.

3 participants