[ferdymercury]

return nan instead of sqrt(abs(value))

Fixes #22755

Was like that since the early beginnigns https://github.com/root-project/root/blame/dd588ebc2c01182af573ea9ac7ce2ddcbc734b6e/treeplayer/src/TTreeFormula.cxx

[github-actions]

Test Results

    23 files      23 suites   3d 14h 12m 36s ⏱️
 3 860 tests  3 858 ✅  0 💤 2 ❌
78 638 runs  78 595 ✅ 41 💤 2 ❌

For more details on these failures, see this check.

Results for commit cb3484a.

♻️ This comment has been updated with latest results.

[guitargeek]

Thank you very much!

What about adding a unit test like this to tree/treeplayer/test/regression.cxx?

// https://github.com/root-project/root/issues/22755
// sqrt() of a negative argument used to evaluate to sqrt(abs(x)) instead of NaN
TEST(TTreeFormulaRegressions, SqrtOfNegative)
{
   TTree t("t", "t");
   int x = 0;
   t.Branch("x", &x, "x/I");
   t.Fill();
   t.GetEntry(0);

   TTreeFormula tf("tf", "sqrt(-4.0)", &t);
   EXPECT_TRUE(std::isnan(tf.EvalInstance()));
   TTreeFormula tf2("tf2", "sqrt(x - 4)", &t);
   EXPECT_TRUE(std::isnan(tf2.EvalInstance()));
   TTreeFormula tf3("tf3", "sqrt(x + 9)", &t);
   EXPECT_FLOAT_EQ(tf3.EvalInstance(), 3.);
}

Also, this is a behavioral change that needs to be explained in the release notes, with an entry like this for example:

#### Behavior change: `sqrt()` of negative arguments in TTreeFormula now returns NaN

Since its introduction in 1995, the formula engine used by `TTree::Draw()`, `TTree::Scan()` and `TTreeFormula`
silently evaluated `sqrt(x)` as `sqrt(abs(x))` for negative arguments (or as `0` in the optimized evaluation path
of the legacy `ROOT::v5::TFormula`). This could produce silently wrong results, e.g. in selections involving
`sqrt` of an expression that can become negative. `sqrt()` now returns NaN for negative arguments, consistent
with `TMath::Sqrt()`, the standard C `sqrt()`, and the modern `TFormula` used by `TF1`.
Note that in a selection, a NaN evaluates as `false`, so entries where the `sqrt` argument is negative now fail
the cut instead of being selected based on `sqrt(abs(x))`.

[ferdymercury]

Thanks Jonas, I added your test and updated RelNotes.
Remaining CI failure seems unrelated.

[pcanal]

Thanks.

[hageboeck]

Hello @ferdymercury,

while I agree that returning wrong values is a bad idea, I think I know why people did it:
When NaN ends up in a histogram, you can't draw it. This should not stop us from fixing the issue, but I think we also need a strategy (maybe initially by creating an issue) to still draw something useful when a NaN is in a histogram.

[ferdymercury]

Thanks Stephan for the feedback!

When NaN ends up in a histogram, you can't draw it.

Hmmm, are you sure? Or what do you mean?
It's well defined / known in ROOT that NaN values end up in the overflow bin.

TTree t("t", "t")
Float_t x;
x = sqrt(-1);
t.Branch("x", &x);
t.Fill();
x = 2;
t.Fill();
x = -5;
t.Fill();
new TCanvas();
t.Draw("x");
new TCanvas();
t.Draw("sqrt(x)");

To me, the second canvas looks currently wrong, since it fills a value at sqrt(|-5|). It looks reasonable to me that this value should disappear into the overflow with the new PR's behavior. The same as if the initial input of x is NaN?

(maybe initially by creating an issue) to still draw something useful when a NaN is in a histogram.

Do you mean a 'boolean' option that does not throw NaNs into the overflow bin but rather on a visible bin ?
Wouldn't it be enough to just call GetXaxis -> SetLimits so that the overflow bin becomes visible ? Or do you mean to have two diferent overflow bins, one for NaNs and one for overflows? A third one for infs ?

In any case, there are many cases such as log() etc that will end up into NaNs, I don't think we should support those kind of things since there are too many edge cases.
If the user wants to show the NaNs really in the histogram, he can do it purposely by writing:

TTree.Draw(sqrt(abs(x))) or log(max(...)) or whatever.
Right?

I think it's always better to make the user work than to automatically/silently fixing too many things on ROOT's side and showing wrong results or hiding issues in your data.

[dpiparo]

/backport to 6.40

[root-project-bot]

Preparing to backport PR #22756 to branch 6.40 requested by dpiparo

[root-project-bot]

Something went wrong with the creation of the PR to backport to 6.40: @dpiparo please see the logs