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

Python 3 #647

Closed
nsmith- opened this issue Feb 18, 2021 · 6 comments
Closed

Python 3 #647

nsmith- opened this issue Feb 18, 2021 · 6 comments
Assignees

Comments

@nsmith-
Copy link
Collaborator

nsmith- commented Feb 18, 2021

As of CMSSW 11_2, ROOT v6.22 is compiled against both python2 and python3. Are there plans to start making Combine python3-compatible?

@nsmith-
Copy link
Collaborator Author

nsmith- commented Feb 19, 2021

It wasn't too much trouble to get the C++ side compiling in 11_2 (see #648 ) so at least python3 users can now use the combine-specific ROOT classes while building their workspace and datacards (my main motivation)

@nucleosynthesis
Copy link
Contributor

nucleosynthesis commented Feb 19, 2021

I remember that Jonas (https://github.com/guitargeek/) had a fork at one point in which the python was migrated to py3 (https://hypernews.cern.ch/HyperNews/CMS/get/higgs-combination/1322.html). I suspect that we could automate quite a lot of the main tools (t2w, python models ...) and then spend some time making sure the various python tools/scripts in test are updated.

@guitargeek
Copy link
Contributor

guitargeek commented Apr 26, 2021

Hi! The fork doesn't exist anymore, because I put what was special about it in my repository with build scripts for Arch Linux:
https://github.com/guitargeek/PKGBUILDs/tree/master/cms-higgs-combine

In the PKGBUILD file you can see what I did for the python 3 migration:

  • format code with autopep8 -i -r . such that python-modernize doesn't hang because of tab/space mixing
  • code formatting with python-modernize -w -n .

Maybe that's helpful!

By the way, since February I'm not in CMS anymore but in the ROOT team, working as the main RooFit developer. If you have any suggestions on how we could improve RooFit upstream to benefit CMS, please don't hesitate to get in touch with me!

I am already using some Higgs combine fit examples to benchmark and optimize RooFit. So for example, you could let me know about some fits that are much slower than they need to be and we could find ways to make them faster. Another thing that might be interesting could be to bring the speed optimizations you made for some pdfs in combine also to RooFit, or integrate some shapes that are widely used in CMS but are missing upstream.

@nsmith-
Copy link
Collaborator Author

nsmith- commented Apr 5, 2022

Pasting here open PRs that touch python code:

@nsmith- nsmith- self-assigned this Apr 27, 2022
@kcormi
Copy link
Collaborator

kcormi commented May 16, 2023

Closing as resolved by #759 . @nsmith- is it worth opening a separate issue for the dictionary ordering (which I think was never addressed?). I guess it shouldn't have any effect, but perhaps there are cases where being able to reproduce the ordering is helpful?

@kcormi kcormi closed this as completed May 16, 2023
@nsmith-
Copy link
Collaborator Author

nsmith- commented May 16, 2023

Dictionary ordering may in principle affect results via numerical precision and instability, but hopefully only in rare instances. Indeed #759 (comment) is not directly addressed at the moment. If we are OK not having consistent behavior between combine v8/10x/py2 and combine v9/py3, then at least going forward things should stay rather consistent due to stable dictionary ordering in py3.7+.

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

No branches or pull requests

4 participants