Skip to content

[hist] Using TKDE::Fill works with empty tkde #14740

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[hist] Using TKDE::Fill works with empty tkde #14740

wants to merge 6 commits into from

Conversation

aaronj0
Copy link
Contributor

@aaronj0 aaronj0 commented Feb 15, 2024 •
edited by ferdymercury
Loading

Fixes #7808

This is happening because in when the parentheses operator overload TKDE::operator()(Double_t x) calls ReInit (const_cast<TKDE*>(this))->ReInit() it returns before setting the fKernel in the case of new data.

One approach is to call SetKernel here:

if (fNewData)  {
      InitFromNewData();
      SetKernel();
      return;
   }

or call it at the end of InitFromNewData().

When that happens, the fKernel is no longer null but this error is reproducible with both iterative options -
With Adaptive we get NaN and Fixed we get inf

This is because the weight calculation is using the nSum that is 0 when binning is not used
Inversing that gives the infinity in the Iteration:Fixed case

This fix:

  • adds the call to SetBinCountData() in InitFromNewData()
  • recreates the kernel fun pointer (previously fKernel ends up a nullptr)
  • calculating nSum as fNEvents if not binning in TKDE::TKernel::operator().

Results:

    auto kde = new TKDE(0, nullptr, 0, 5, "KernelType:Gaussian;Iteration:Adaptive;Mirror:noMirror;Binning:RelaxedBinning", 1);
    for (unsigned int i = 0; i < 100; i++) { kde->Fill(gRandom->Gaus(2,1)); }
    std::cout<<kde->GetValue(2)<<"\n";

Gives
0.487581

@aaronj0 aaronj0 requested a review from lmoneta as a code owner February 15, 2024 13:32
@phsft-bot
Copy link

Can one of the admins verify this patch?

lmoneta
lmoneta approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing the TKDE when updating the data with Fill.

I agree the SetKernel() is needed in the InitFromNewData function, since by adding the data both the fixed bandwidth and the adaptive one needs to be re-computed.
A possible optimisation would be to have a function UpdateKernel, which will re-use the existing kernel, but just changing the fixed bandwidth or the adaptive one, depending on the case.

@lmoneta
Copy link
Member

lmoneta commented Feb 15, 2024

@phsft-bot build

@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2004/python3.
See console output.

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

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

Failing tests:

lmoneta
lmoneta previously requested changes Feb 15, 2024
View reviewed changes
hist/hist/src/TKDE.cxx Outdated
@@ -1015,8 +1023,7 @@ Double_t TKDE::TKernel::operator()(Double_t x) const {
Bool_t useCount = (fKDE->fBinCount.size() == n);
// also in case of unbinned unweighted data fSumOfCounts is sum of events in range
// events outside range should be used to normalize the TKDE ??
Double_t nSum = fKDE->fSumOfCounts; //(useBins) ? fKDE->fSumOfCounts : fKDE->fNEvents;
//if (!useCount) nSum = fKDE->fNEvents;
Double_t nSum = (useCount) ? fKDE->fSumOfCounts : fKDE->fNEvents;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is causing a test to fail. The normalisation of the TKDE is wrong, because now nSum is (in case of unbinned, useCount=false) computed from all events, instead it should be computed from all the events in the range (see SetBinCounts).
This line has to be reverted as before.

We could add a warning (or error) , maybe better in SetKernel so it is not for every event when fKDE->fSumOfCounts is zero, so we avoid having inf values

@phsft-bot
Copy link

Build failed on ROOT-performance-centos8-multicore/soversion.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

Build failed on mac12arm/cxx20.
Running on 194.12.161.128:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 16, 2024 •
edited
Loading

Test Results

    18 files      18 suites   4d 8h 6m 48s ⏱️
 2 678 tests  2 676 ✅ 0 💤 2 ❌
46 484 runs  46 481 ✅ 0 💤 3 ❌

For more details on these failures, see this check.

Results for commit cf18add.

♻️ This comment has been updated with latest results.

@ferdymercury
[hist] revert normalization
608bcbc 
as suggested by lmoneta. This was causing a test to fail. The normalisation of the TKDE was wrong, because now nSum was (in case of unbinned, useCount=false) computed from all events, instead it should be computed from all the events in the range (see SetBinCounts).
@ferdymercury
[hist] raise warning with zero sum
3b41337 
as suggested by lmoneta
@ferdymercury
[hist] add test for regression
95d3744
@ferdymercury
[hist] missing delta in test
3d8beaa
@ferdymercury
[hist] adapt test delta
cf18add
@ferdymercury ferdymercury added the 1st Hackathon: the Fixhathon This issue can be tackled at a ROOT fixathon label Dec 18, 2024
@ferdymercury ferdymercury requested a review from dpiparo December 19, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmoneta lmoneta Awaiting requested review from lmoneta lmoneta is a code owner

@guitargeek guitargeek Awaiting requested review from guitargeek

@dpiparo dpiparo Awaiting requested review from dpiparo

At least 1 approving review is required to merge this pull request.

Assignees

@lmoneta lmoneta

Labels
1st Hackathon: the Fixhathon This issue can be tackled at a ROOT fixathon in:Hist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug in TKDE::Fill
4 participants
@aaronj0 @phsft-bot @lmoneta @ferdymercury