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

[v626][RF] Backports of RooFit PRs to v6-26-00-patches: Part 18 #11057

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Jul 26, 2022

This is a backport of all the relevant bugfix RooFit PRs that were recently merged to master to v6-26-00-patches (in the right order, to not have the commit history diverge too much).

  1. [RF] Cleanup RooLagrangianMorphFunc #9912
  2. [RF] Fixup to parameter index calculation in ParamHistFunc #10989
  3. [RF] Skip out-of-range events in RDataFrame to RooDataSet Helper #11018
  4. [RF] Enable streaming of RooLagrangianMorphFunc and other improvements #11023

rahulgrit and others added 6 commits July 27, 2022 00:43
Last year, with commit 3657e7c, the parameter index calculation was
changed to be on the fly instead of using a look-up map, which is much
faster.

However, the implemented formula was not correct for two or three
dimensions, which is fixed by this commit.

To make sure that the index computation is correct this time, the new
code was tested in this code snippet with various inputs:

```C++
void runTest(int nx = 42, int ny = 42, int nz = 42) {
  const int nxy = nx * ny;
  const int nyz = ny * nz;

  for (int i = 0; i < nx; ++i) {
    for (int j = 0; j < ny; ++j) {
      for (int k = 0; k < nz; ++k) {
        const int index = k + j * nz + i * ny * nz;
        const int gammaIndex = i + j * nx + k * nx * ny;

        const int i2 = index / nyz;
        const int tmp = index % nyz;
        const int j2 = tmp / nz;
        const int k2 = tmp % nz;

        const int gammaIndex2 = i2 + j2 * nx + k2 * nxy;

        if (gammaIndex2 != gammaIndex) {
          std::cout << "The unraveled indices were not correct!"
                    << std::endl;
          return;
        }
      }
    }
  }
}
```

Needs to be backported to the 6.26 branch to get into the 6.26.06 patch
release.

This commit the following problem reported on the forum:
https://root-forum.cern.ch/t/cpycppyy-segfault-on-mac-m1/50822
The RDataFrameHelper should be consistent with creating a RooDataSet
from a TTree, meaning out-of-range events should be skipped. This is
implemented in this commit, borrowing the logic from
`RooTreeDataStore::loadValues()`. A unit test is also implemented.

The previous logic of just taking just all values to fill the dataset
was very dangerous, because these values then clipped to the RooRealVar
limits and biased the number of events observed at the boundaries.

Closes root-project#11017.
This commit removes the `_curNormSet` and `_ownParameters` member
variables from the RooLagrangianMorphFunc. The `_ownParameters` was
simply unused, and the `_curNormSet` was redundant because there is
already `RooAbsReal::_lastNSet` that also points to the current
normalization set.

Now, the `RooLagrangianMorphFunc::getValV()` can also be removed because
the sole purpose was to set `_curNormSet`, and also the private
`RooLagrangianMorphFunc::getCache()` interface can be changed to take no
normSet parameter (as it was unused anyway).

The `RooLagrangianMorphFunc::_cacheMgr` declaration is also moved to the
bottom of the file together with the other member variables.
This is to enable IO for the RooLagrangianMorphFunc.
The title for the x-axis of the first plot should be `"p_{T}^{V}"` and
not `"c_{Hq^{(3)}}"`.

Also, the title for the observable is changed to `"p_{T}^{V}"` to get
the right axis label for the x-axes of the plots.
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

  • [2022-07-26T23:56:56.549Z] C:\build\workspace\root-pullrequests-build\build\bin\libRooFit.dll : fatal error LNK1120: 3 unresolved externals [C:\build\workspace\root-pullrequests-build\build\roofit\roofit\RooFit.vcxproj]

@guitargeek guitargeek force-pushed the v6-26-00-patches_roofit_backports_18 branch from 3b9183b to e7ac26b Compare July 27, 2022 05:11
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

@guitargeek guitargeek merged commit 256c7b2 into root-project:v6-26-00-patches Jul 27, 2022
@guitargeek guitargeek deleted the v6-26-00-patches_roofit_backports_18 branch July 27, 2022 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants