Skip to content

JsMVA: use latest JSROOT, use /static path to load code #12484

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

linev
Copy link
Member

@linev linev commented Mar 14, 2023

Fixes #12441

  1. Move all JS/CSS files to js/mva/ folder which can be accessed via /static/mva/ URL from Jupyter Notebook. This allows to use JsMVA code directly from ROOT without need to rely on https://root.cern/js/jsmva/latest
  2. Remove minified scripts - no real gain in performance, only extra step in deployment
  3. Adjust JsMVA.js code to JSROOT v7. Use bundle provided in build/jsroot.js.
  4. Adjust OutputTransformer.py to work with python3, probably other scripts should be improved as well
  5. Adjust NetworkDesigner.js to use d3 v6, load d3 from external source

Still to do:

  1. Fix more problems with python3 - see attached image from my notebook
    python3_error
  2. Fix NeuralNetwork.js to be usable with latest d3.js. I need working example for it.

@linev linev requested a review from lmoneta March 14, 2023 12:37
@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Test Results

    10 files      10 suites   2d 5h 40m 24s ⏱️
 2 637 tests  2 637 ✅ 0 💤 0 ❌
24 906 runs  24 906 ✅ 0 💤 0 ❌

Results for commit 9624d9a.

♻️ This comment has been updated with latest results.

guitargeek and others added 5 commits May 22, 2024 04:38
While JSROOT has absolutely different modules structure,
just try to use build.js bundle. It also include d3_select,
which can be reused.
Like JSROOT, let use /static path which provided by Jupyter notebooks
Avoid usage of minified scripts - this reduce number of steps
needed to deploy JsMVA. Use d3 from external location
This let access them from Jupyter Notebooks via
/static/mva/ path.

Remove minified script to simplify deplyment

Add missing jquery scripts
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Thank you very much! I tested that JsMVA works the same way before and after this PR 👍 Let's merge this.

I helped you reviving this PR because it was my fault that there were conflicts: I have moved all the files around without giving you the opportunity to merge this PR first 🫤

@linev
Copy link
Member Author

linev commented May 22, 2024

@guitargeek

Are all python3 problems resolved now?
Did you try NeuralNetwork? It may need adjustments for d3 v6 which used now

@guitargeek
Copy link
Contributor

guitargeek commented May 22, 2024

No, I didn't try anything besides the unit tests in roottest (which are actually deactivated in the CMakeLists.txt).

What do you mean with Python 3 problems? We don't support Python 2 anymore, so all code only needs to run in Python 3. Which is does, right? Sorry, maybe I didn't get the problem.

If you mean with Python 3 problems the error you have screenshot: I get the same error also without your PR.

So given that JsROOT is not working very well and no user complained, I would not spend more time on this and suggest to merge this PR and close the issue.

@linev
Copy link
Member Author

linev commented May 23, 2024

This commit b9d085e is adjusting to python3. I am not sure if changes in other scripts is necessary.

So given that JsROOT is not working very well and no user complained, I would not spend more time on this and suggest to merge this PR and close the issue.

Here problem not jsroot but jsMVA. I have no tests to check functionality of NeuralNetwork. Most probably it will not work after merging PR. Therefore I prefer test it before merging.

@@ -23,7 +23,7 @@ def __init__(self, shell):
@magic_arguments()
@argument("arg", nargs="?", default="on", help="Enable/Disable JavaScript visualisation for TMVA")
def jsmva(self, line):
from JsMVA.JPyInterface import functions
from ROOT.JsMVA.JPyInterface import functions
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fixed in master branch (using a relative import):
from .JPyInterface import functions

@@ -19,7 +19,7 @@
paths: {
"jquery-connections": baseURL + "jquery.connections.min",
"jquery-timing": baseURL + "jquery-timing.min",
"d3": "/static/scripts/d3.min"
"d3": "https://d3js.org/d3.v6.min"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this us the module from jsroot?
"/static/modules/d3.mjs "

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not think that ES6 module can be used here.
One have to use alternative location

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JsMVA should be updated to use the same jsroot version as the rest of ROOT
5 participants