-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[DF][Fix root-project#9117] Applied aliases in tutorials #9205
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
Conversation
|
Starting build on |
eguiraud
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ivan, looks great! Can merge after the spurious file tutorials/dataframe/RVecF is removed 😄
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
Failing tests:
|
|
Starting build on |
eguiraud
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like there are some errors to be fixed, mainly due to the fact that RVec<T> is available both in namespace ROOT::VecOps and in namespace ROOT, but the aliases are only available in namespace ROOT.
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
Failing tests:
|
|
Build failed on mac11.0/cxx17. Errors:
Failing tests:
|
|
Starting build on |
|
Build failed on mac11.0/cxx17. Failing tests: |
|
Build failed on windows10/cxx14. Failing tests: |
eguiraud
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just needs a last pass
tutorials/dataframe/df016_vecOps.C
Outdated
|
|
||
| using ROOT::RDataFrame; | ||
| using namespace ROOT::VecOps; | ||
| //using ROOT::RDataFrame; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here and below, must remove commented code
tutorials/dataframe/df016_vecOps.py
Outdated
| coordDefineCode = '''using namespace ROOT; | ||
| RVecD {0}(len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| coordDefineCode = '''using namespace ROOT; | |
| RVecD {0}(len); | |
| coordDefineCode = '''ROOT::RVecD {0}(len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the other remark were resolved.
|
Starting build on |
c2a6bdf to
58cd622
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
Failing tests: |
|
Build failed on mac11.0/cxx17. Errors:
Failing tests: |
|
Build failed on windows10/cxx14. Failing tests: |
|
Starting build on |
eafa8af to
a67763a
Compare
|
Starting build on |
|
Build failed on mac11.0/cxx17. Errors:
|
| auto df_ge2el2mu = df.Filter("nElectron>=2 && nMuon>=2", "At least two electrons and two muons"); | ||
| auto df_eta = df_ge2el2mu.Filter("All(abs(Electron_eta)<2.5) && All(abs(Muon_eta)<2.4)", "Eta cuts"); | ||
| auto pt_cuts = [](rvec_f mu_pt, rvec_f el_pt) { | ||
| auto pt_cuts = [](RVecF mu_pt, RVecF el_pt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here and below, this is now copying a lot of arrays as they are being passed by value. it should be const RVecF & everywhere, or at least RVecF &
| using VecI_t = const ROOT::RVec<int>&; | ||
| bool GoodElectronsAndMuons(VecI_t type, VecF_t pt, VecF_t eta, VecF_t phi, VecF_t e, VecF_t trackd0pv, VecF_t tracksigd0pv, VecF_t z0) | ||
| using namespace ROOT; | ||
| bool GoodElectronsAndMuons(RVecI type, RVecF pt, RVecF eta, RVecF phi, RVecF e, RVecF trackd0pv, RVecF tracksigd0pv, RVecF z0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const RVec &, this needlessly copies a lot
| # Compute invariant mass of the four lepton system and make a histogram | ||
| ROOT.gInterpreter.Declare(""" | ||
| float ComputeInvariantMass(VecF_t pt, VecF_t eta, VecF_t phi, VecF_t e) | ||
| float ComputeInvariantMass(RVecF pt, RVecF eta, RVecF phi, RVecF e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const RVecF &
| using VecI_t = const ROOT::RVec<int>&; | ||
| int FindGoodLepton(VecI_t goodlep, VecI_t type, VecF_t lep_pt, VecF_t lep_eta, VecF_t lep_phi, VecF_t lep_e, VecF_t trackd0pv, VecF_t tracksigd0pv, VecF_t z0) | ||
| using namespace ROOT; | ||
| int FindGoodLepton(RVecI goodlep, RVecI type, RVecF lep_pt, RVecF lep_eta, RVecF lep_phi, RVecF lep_e, RVecF trackd0pv, RVecF tracksigd0pv, RVecF z0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const RVecF &
Ah correct, I am realizing the const now. Updating in a tiny bit. |
|
Starting build on |
1 similar comment
|
Starting build on |
730001f to
e832ce5
Compare
|
Starting build on |
|
Passing |
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
Failing tests: |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Failing tests: |
|
Build failed on mac11.0/cxx17. Errors:
Failing tests:
|
|
Starting build on |
3447e9b to
3eb850a
Compare
|
Starting build on |
|
Build failed on mac11.0/cxx17. Failing tests: |
eguiraud
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, code indentation is weird in some places, you can run git clang-format to fix just the formatting of your patch (rather than of all the file), then we are good 🎉
|
can you be more specific @eguiraud , |
|
Starting build on |
Make use of the aliases RVecI, RVecD, RVecF in the tutorials. Note that these aliases are present in the ROOT namespace, i.e. ROOT::RVecI.
24eca34 to
7f797cb
Compare
|
Starting build on |
eguiraud
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 will merge when the CI run is done
|
Build failed on mac11.0/cxx17. Failing tests: |
This Pull request: Applied aliases in tutorials for RVec (int, float, double)
Changes or fixes: Use newer aliases
Checklist:
This PR fixes #9117