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

[RF] Pythonisations for RooFit #7217

Closed
8 tasks
hageboeck opened this issue Feb 15, 2021 · 11 comments
Closed
8 tasks

[RF] Pythonisations for RooFit #7217

hageboeck opened this issue Feb 15, 2021 · 11 comments

Comments

@hageboeck
Copy link
Member

Many C++-like workflows in RooFit can be beautified on the Python side.
Currently:

pdf.fitTo(data, ROOT.RooFit.Range("sideband"))

Propose:

pdf.fitTo(data, range="sideband")

Overall, this is not difficult once one figures this out for the first function. Good for e.g. a summer student project. A few candidates that could be pythonised:

  • RooAbsPdf::fitTo
  • RooAbsPdf::chi2FitTo
  • RooAbsReal::plotOn
  • RooAbsPdf::createNLL
  • RooDataHist::RooDataHist
  • RooDataSet::RooDataSet
  • RooAbsPdf::paramOn
  • ...
@hcmidt
Copy link
Contributor

hcmidt commented Feb 24, 2021

Hi,

I am new to root and trying to wrap my head around some of its features. I thought a good place to start would be to work on this issue. I started by pythonizing fitTo and I'd like to send you a pull request. I take it that I should provide some tests for the feature, but I could not find any resources regarding the testing conventions in python for root. If there are any specific guidelines, please let me know, otherwise I'll just send you a pull request and you can take a look if it is really necessary.

Cheers

@hageboeck
Copy link
Member Author

Hi @hcmidt,

I'm not working on RF, any more, but it's great to see someone offering help!

Regarding tests, see here:
https://github.com/root-project/root/blob/master/bindings/pyroot/pythonizations/test/roodatahist_ploton.py

In this folder, you have all the tests for the Pythonisations. If you have something, include a test here (I guess it makes sense to create a new file for RooAbsPdf etc. @etejedor is the expert on PyROOT, so make sure to request him as a reviewer when there's something to look at. And request @guitargeek for the RooFit part as well!

@hageboeck
Copy link
Member Author

@hcmidt
Copy link
Contributor

hcmidt commented Mar 1, 2021

Thanks @hageboeck, the pull request is now open.

@guitargeek
Copy link
Contributor

To collect some info in this issue, here is the link to an interesting post in the forum that shows an example for why pythonization of the RooDataSet is really needed:

https://root-forum.cern.ch/t/combining-roodatasets-in-pyroot/43615

@guitargeek
Copy link
Contributor

Another Pythonization idea that occured to me in the PR reviews: implement += for RooArgLists.

So instead of doing this:

l = ROOT.RooArgList()
l.add(x)
l.add(y)

One can also do this, just like with Python lists:

l = ROOT.RooArgList()
l += [x, y]

@etejedor
Copy link
Contributor

l += [x, y]

Would it be also interesting to implement l1 + l2 ?

@guitargeek
Copy link
Contributor

guitargeek commented Jun 14, 2022

The pythonizations suggested in this issue have been implemented, so it can be closed. For any further specific RooFit pythonization ideas, please open a new issue or a PR directly.

The Pythonization of RooArgLists that I suggested before is not so useful after all, because now that RooArgLists can be replaced by Python lists to begin with it would be redundant to reimplement the list behavior.

@root-project root-project deleted a comment from github-actions bot Oct 3, 2022
@root-project root-project deleted a comment from github-actions bot Oct 3, 2022
@root-project root-project deleted a comment from github-actions bot Oct 3, 2022
@guitargeek guitargeek reopened this Oct 3, 2022
@guitargeek
Copy link
Contributor

guitargeek commented Oct 3, 2022

Reopened, because there are new ideas for Pythonizations mentioned in this comment:
#11421 (comment)

To quote that comment:

  1. sometimes I would have found useful to be able to pass python number anywhere a RooAbsReal is required, although I suspect this may require a pythonization for each pdf.
  2. RooSimultaneous map constructor does not accept a python dictionary yet
  3. one thing that surprised me a couple of times at the beginning is that RooAbsArg does not keep its servers alive from the python GC so you actually need the same workarounds as in C++ (importing frequently to a workspace).
    However, I suspect that if they did keep servers alive, server redirection would likely lead to desync between the C++ and python views of the graph.

The third one is now implemented in #11500, which also got backported to 6.26.08

@guitargeek
Copy link
Contributor

Another idea we discussed in the RooFit developers meeting: often a RooLinkedList is used to pass around RooFit command arguments, so it would be nice if it could be implicitly created from a Python dictionary to be used in any function that takes a RooLinkedList.

@guitargeek
Copy link
Contributor

The remaining Pythonization suggestions in my last two posts were either implemented or deemed not relevant enough to do so. I'll re-close this issue.

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

No branches or pull requests

4 participants