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

Jsmva issues #1520

Merged
merged 3 commits into from Aug 1, 2018
Merged

Jsmva issues #1520

merged 3 commits into from Aug 1, 2018

Conversation

ellert
Copy link
Contributor

@ellert ellert commented Jan 16, 2018

The Fedora Packaging Guidelines some time ago added a section on packaging javascript files. This section says that minified scripts should be treated as compiled code. And like for programs written in other languages, pre-compiled code that is part of the sources must not be used in packaging, but must be regenerated from sources during the package build. I recently implemented these guidelines in the root package and found some issues.

This pull request has three commits.

  1. The word "default" is a reserved word in the language, and when used as a key in a dictionary it must be quoted. The yuicompressor minifier complains about this:
$ yuicompressor etc/notebook/JsMVA/js/DecisionTree.js 
[ERROR] in etc/notebook/JsMVA/js/DecisionTree.js
  57:24:invalid property id
  1. In one place (and one place only) in the script a construct from a very new version of the javascript standard is used for defining default values to function parameters. This unnecessarily makes the code incompatible with old browsers. It also makes it hard to run the minification during the package build as both yuicompressor and uglufyjs choke on it.
$ yuicompressor etc/notebook/JsMVA/js/NeuralNetwork.js 
[ERROR] in etc/notebook/JsMVA/js/NeuralNetwork.js
  509:75:missing ) after formal parameters
$ uglifyjs etc/notebook/JsMVA/js/NeuralNetwork.js  -c -m
Parse error at etc/notebook/JsMVA/js/NeuralNetwork.js:509,73
 = function (divid, netobj, oldStructure=false) {
                                        ^
SyntaxError: Unexpected token operator «=», expected punc «,»
  1. The code has not been updated to reflect changes in jsroot. The code still tries to load d3.v3.min.js from jsroot. In the current version of jsroot the d3 script has been updated to version 4 and the file is now called d3.min.js. The pull request changes the name of the file in the two places where it is referenced, so it should now find the file. However this change is incomplete since the d3 API changes between the versions and some porting is needed.

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@pcanal
Copy link
Member

pcanal commented Jan 19, 2018

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on centos7/gcc49, mac1013/native, slc6/gcc49, slc6/gcc62, slc6/gcc62, ubuntu16/native, ubuntu16/native, windows10/vc15 with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on mac1013/native.
See console output.

Warnings:

  • /cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1013-clang90-opt/lib/R/include/R.h:40:10: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
  • /cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1013-clang90-opt/lib/R/include/R_ext/RS.h:29:10: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
  • /cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1013-clang90-opt/lib/R/include/Rinternals.h:60:11: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
  • /cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1013-clang90-opt/lib/R/library/Rcpp/include/Rcpp/r/headers.h:57:10: warning: non-portable path to file '<RVersion.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
  • /cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1013-clang90-opt/lib/R/include/R_ext/Visibility.h:29:10: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
  • /cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1013-clang90-opt/lib/R/include/R.h:40:10: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
  • /cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1013-clang90-opt/lib/R/include/R_ext/RS.h:29:10: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
  • /cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1013-clang90-opt/lib/R/include/Rinternals.h:60:11: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
  • /cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1013-clang90-opt/lib/R/library/Rcpp/include/Rcpp/r/headers.h:57:10: warning: non-portable path to file '<RVersion.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
  • /cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1013-clang90-opt/lib/R/include/R_ext/Visibility.h:29:10: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]

And 10 more

Failing tests:

Copy link
Member

@Axel-Naumann Axel-Naumann left a comment

Choose a reason for hiding this comment

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

Up to @lmoneta to decide.

@etejedor
Copy link
Contributor

@ashlaban among the fixes of this PR, there is one related to the d3 issue in JsMVA you are currently fixing. Can you have a look? Perhaps you can also incorporate the other fixes.

@ashlaban
Copy link
Contributor

Thanks for the catch. Actually I don’t understand why this PR is not integrated. It seems to be a more extensive version of my change.

I will update my PR with the d3 name change I missed and discuss with Lorenzo how to best respond to this PR.

@ashlaban
Copy link
Contributor

For reference: Parts of these changes are included in PR#2353.

@ashlaban
Copy link
Contributor

ashlaban commented Aug 1, 2018

As @ellert points out, we need to port jsmva to d3 v4 that jsroot uses. Right now jsmva is basically unusable; Most of it's functionality depends on d3. Incorporating this PR will allow most of jsmva functionality to run, but a few things will be missing due to an api change.

I suggest to integrate this PR as is and provide the update to the rest of jsmva in a separate PR (unless you @ellert would volunteer to update this PR).

Details (and for future reference):
There are a few places of jsmva that should be updated to run under d3.v4. The swan example
https://cern.ch/swanserver/cgi-bin/go?projurl=https://raw.githubusercontent.com/iml-wg/tmvatutorials/master/TMVAinJupyter_FullTutorial.ipynb shows what is broken.

The problems arise when calling:

  • BookDNN (interactive design of DNN's; does not render)
  • DrawNerualNetowork (visualising NN's; does not render)
  • DrawDecisionTree (visualising BDT's; does not render)
  • DrawCutEfficiencies (curves come up in grayscale, should be in color)

List of changes for 3 -> 4 can be found here: https://github.com/d3/d3/blob/master/CHANGES.md#changes-in-d3-40. Important ones to note are:

  • d3.svg.line -> d3.line
  • d3.svg.diagonal -> removed

@lmoneta
Copy link
Member

lmoneta commented Aug 1, 2018

I think then we can finally merge this now, and having further changes in a separate PR.
Sorry for having missed this one

@lmoneta lmoneta merged commit 7c1dae6 into root-project:master Aug 1, 2018
@ellert ellert deleted the jsmva-issues branch August 2, 2018 11:40
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.

None yet

7 participants