I am experiencing the exact same problem as here. As the thread died off pretty quickly I am not sure whether it has been left unanswered of fixed in a private channel.
To summarize my problem, I am using RooMomentMorph in PyRoot with O(50) histograms of 11 bins and in a loop I use the setVal and createHistogram many times. Unfortunately this makes the code really slow and the memory blows up relatively fast. As the previous poster suspected I assume some junk is not deleted at some point. On the other hand the problem disappears when I set useHorizontalMorphing to false, and even though the code still seem to work I am afraid to be missing some key components on the method.
At the risk of being redundant with the previous post, but as I could not find a fix anywhere, could any expert have a look at this issue ?
I can of course provide a minimal example to reproduce in case needed.
good news, I found the memory leak and opened a PR to fix it!
So the leak will be gone for the upcoming ROOT 6.26 and I will probably also backport it to 6.24. Please let me know if you need to get this fixed also in older releases.
About the speed: the moment morphing is intrinsically slow in RooFit, but you can do some things in your script to speed it up. There is one obvious thing that comes to my mind.
Right now you are creating all these “true hists” redundantly for each reco hist. You can create them all only once before looping over the reco hists, and then only do the chi2 tests in the recoHist loop.
This will already give you a huge speedup. Let me know if this is fast enough, if not I also have more ideas if you want.
I have translated your example to C++ because it’s easier to find memory leaks like this, and I also implemented the speedup suggestion I just mentioned. I upload it here in case it’s useful for you: script_fast.C (2.8 KB)
Please let me know if something is unclear of if you have further questions!
Many thanks for this very quick fix ! When can I expect this version to be released ? As I am working on my institute cluster I can hardly tinkle with a local installation but I will ask our IT when it becomes available.
Regarding the speedup tip, you are completely right I had not thought about that for this simple example. However I had simplified the logic to make your life easier, in practice I used two scans : a coarse one from which the minimum is used to perform a finer scan. Regardless, your idea still makes sense, especially for the coarse scan whose coordinates are hardcoded, so thanks for that !
If you have more ideas I would of course be very happy to learn about them.
Hi, the memory leak is now fixed in ROOT master and in 6.24. The next ROOT point release will probably happen in a few weeks, so if you convince your IT support to install 6.24/02 when it’s out you can benefit from it.
Concerning the other speedup idea I have: the RooAbsReal::createHistogram function has a lot of overhead because it clones the pdf and some of the intermediate cashed results have to be recomputed.
That means createHistogram is really not good for repeated use with the same pdf. I don’t know the best way to improve this at the moment, but I’d suggest for your usecase that you fill the histogram manually, like I do in this code example: script_fast_manual.C (3.9 KB)
I hope this is useful for you, please don’t hesitate to ask further questions!
I had already implemented a cache for the histograms to avoid some costly recomputations but the speed-up from your trick is impressive, a few orders of magnitude faster (even considering I still have the memory leak from an old root version) !
One small question though, why the 2 in this line ?
Oh, I’m very sorry! I was just putting this to check if the Chi2 test normalizes the histograms itself or not. Please remove this factor 2 as it gives the wrong results! I’ll update also the script in my post to not do any further harm should someone else use that code.