[TFormula] TFormula: Pol functions do not accept variable name as arguments #17623

Aditya-138-12 · 2025-02-05T11:59:47Z

This Pull request:

Add a new BuildPolynomial method to build the polynomials.
Modify HandlePolN's argument passing to extract everything between the brackets and store it in tokens (previously, it only allowed numeric).
Add tests for the functionality in a new file named test_TFormulaPol.cxx in hist/hist/test.

Changes or fixes:

Allow variables in TFormula Pol functions like
TF1 f1("f1", "pol1(x, 0)");
TF1 f1("f1", "pol1(x, [A], [B])");

Checklist:

[✅] tested changes locally
[❌] updated the docs (if necessary)

This PR fixes #16794

, 1. Added a New BuildPolynomial method to build the polynomials, 2. Changes in HandlePolN's argument passing, previously the code allowed only numeric, but in the new version extracts everything between the brackets and store it in the tokens, 3. Also added the tests for the functionality in the new file named test_TFormulaPol.cxx

hageboeck

Hello @Aditya-138-12, and thanks for picking up the issue!

I have some questions before I will be able to review the code (because I may have found a bug, see below 🙂).

Could I ask you to format the commit message in this way:

Short description of the commit.

<More text>

This will improve how github and git will display the commit once it's merged.

Then, let's discuss the questions below, review the changes in TFormula, and see to getting your work merged. 👍

hageboeck · 2025-02-06T12:12:59Z

hist/hist/test/test_TFormulaPol.cxx

+    TestSupport::CheckDiagsRAII diags;
+
+    TFormula f1("f1", "pol1");
+    EXPECT_EQ(f1.GetExpFormula(), TString("([p0]+[p1]*[p0]+[p1]*x)"));


I'm not sure this is correct. Currently, this yields:
([p0]+[p1]*x)

So could this be a bug?

Yeah,
I think I’ve messed up the functionality a bit. Shouldn't have changed the maintainers' code so much. My bad!

No problem! You could proceed by first putting the expected strings in the tests, and then you can work through the code to make the test pass.

hageboeck · 2025-02-06T12:13:11Z

hist/hist/test/test_TFormulaPol.cxx

+    EXPECT_EQ(f1.GetExpFormula(), TString("([p0]+[p1]*[p0]+[p1]*x)"));
+
+    TFormula f2("f2", "pol2");
+    EXPECT_EQ(f2.GetExpFormula(), TString("([p0]+[p1]*[p0]+[p1]*x+[p2]*TMath::Sq(x))"));


Currently, this yields:
([p0]+[p1]*x+[p2]*TMath::Sq(x))

Yeah,
I’ll work on fixing the incorrect outputs. Can I comment on this thread if I need any help regarding the same?

hist/hist/test/test_TFormulaPol.cxx

hageboeck · 2025-02-06T12:16:21Z

hist/hist/test/test_TFormulaPol.cxx

+};
+
+TEST(TFormulaPolTest, BasicPolynomialConstruction) {
+    TestSupport::CheckDiagsRAII diags;


This and the below instances can be removed if the formula doesn't generate any warnings or errors.

hageboeck · 2025-02-06T12:18:15Z

hist/hist/test/CMakeLists.txt

@@ -26,6 +26,7 @@ ROOT_ADD_GTEST(testMapCppName test_MapCppName.cxx LIBRARIES Hist Gpad)
 ROOT_ADD_GTEST(testTGraphSorting test_TGraph_sorting.cxx LIBRARIES Hist)
 ROOT_ADD_GTEST(testSpline test_spline.cxx LIBRARIES Hist)
 ROOT_ADD_GTEST(testTF1Simple test_tf1_simple.cxx LIBRARIES Hist RIO)
+ROOT_ADD_GTEST(testTFormulaPolN test_TFormulaPol.cxx LIBRARIES Core Hist)


I would suggest to add all your tests directly in hist/hist/test/test_TFormula.cxx

hageboeck · 2025-02-06T12:20:13Z

hist/hist/src/TFormula.cxx

      polPos = formula.Index("pol");
   }
 }

+TString TFormula::BuildPolynomial(const TString& var, Int_t degree, Int_t startParam)


Could you add a doxygen documentation to this function? A short explanation as to what it does and what the arguments mean should be sufficient.

Is this the correct reference to learn more about Doxygen.
Also can you please help me with this?

I can help by giving you an example of a well-documented function:
Here is an example from RooFit. This is how it looks in doxygen:
https://root.cern.ch/doc/master/classRooRealVar.html#adb1f0743490454b07104503dddc49544

And this is how it looks in the code:

root/roofit/roofitcore/src/RooRealVar.cxx

Lines 375 to 381 in 9b16969

////////////////////////////////////////////////////////////////////////////////

/// Create a uniform binning under name 'name' for this variable.

/// \param[in] nBins Number of bins. The limits are taken from the currently set limits.

/// \param[in] name Optional name. If name is null, install as default binning.

void RooRealVar::setBins(Int_t nBins, const char* name) {

setBinning(RooUniformBinning(getMin(name),getMax(name),nBins),name);

}

Essentially, a good comment has:

Three ///, so doxygen parses it.

One sentence for a brief description (ending with a dot)

More sentences to describe the function if necessary.

And a description for each parameter: \param <parameterName> This parameter does xxx

I know that the old code in ROOT doesn't have this yet, but we are trying to improve on the old ways. 🙂 That's where you come in!

Thanks a lot for the example! This helps a lot. I'll definitely follow this structure for documenting my function 👍

hageboeck

Hello, thanks for getting back!

Please find below a few comments. Most importantly, I think we need to extend two of the tests, because named parameters are not covered yet, but this seems to be the most important part of the issue description.

hageboeck · 2025-02-10T12:32:57Z

hist/hist/src/TFormula.cxx

+///    Extended Code Documentation
+///    The polynomial handling has been enhanced to support explicit variable names
+///    and parameter placeholders in polynomial expressions. This makes the syntax
+///    more consistent with other function types in ROOT (like gaus) and provides
+///    more flexibility in polynomial construction.
+///    The function reports errors through ROOT's error handling system
+///
+///    This documentation update reflects changes made to support
+///    GitHub issue #16794 for enhanced polynomial syntax support.


This kind of information should be in the commit message.

hageboeck · 2025-02-10T12:37:55Z

hist/hist/src/TFormula.cxx

+///    Extended Syntax:    polN(var, M) - Polynomial of degree N with variable var and parameters starting at [M]
+///                        polN(var, [A]) - Polynomial of degree N with variable var and parameters starting at [A]


The function(x, [A]) syntax normally refers to parameters with names, so you could e.g. do:
formula.SetParameter("A", 4.);

This should be reflected in the documentation. Could you also add a test for this, please?

Suggested change

/// Extended Syntax: polN(var, M) - Polynomial of degree N with variable var and parameters starting at [M]

/// polN(var, [A]) - Polynomial of degree N with variable var and parameters starting at [A]

/// Extended Syntax: polN(var, M) - Polynomial of degree N with variable var and parameters starting at M

/// pol1(var, [A], [B]) - Polynomial of degree 1 with variable var and parameters named "A" and "B"

Thank you for pointing this out! I’ll update the documentation to reflect this behavior and add a test case accordingly. I’ll submit the changes soon.

hageboeck · 2025-02-10T12:42:12Z

hist/hist/test/test_TFormula.cxx

+    TFormula f1("f1", "pol1(x,[5])");
+    EXPECT_EQ(f1.GetExpFormula(), TString("([p5]+[p6]*x)"));


Could you extend this test to check that parameters can be reached by name?
This is probably the most important test in this feature.

Suggested change

TFormula f1("f1", "pol1(x,[5])");

EXPECT_EQ(f1.GetExpFormula(), TString("([p5]+[p6]*x)"));

TFormula f1("f1", "pol1(x,[A])");

EXPECT_EQ(f1.GetExpFormula(), TString("([p5]+[p6]*x)"));

f1.SetParameter("A", -1.234);

f1.SetParameter(6, -1.235);

EXPECT_EQ(f1.GetParameter("A"), -1.234);

EXPECT_EQ(f1.GetParameter(5), -1.234);

EXPECT_EQ(f1.GetParameter(6), -1.235);

f1.SetParameter(5, -1.236);

EXPECT_EQ(f1.GetParameter("A"), -1.236);

EXPECT_EQ(f1.GetParameter(5), -1.236);

EXPECT_EQ(f1.GetParameter(6), -1.235);

Could you write a similar test pol2(y, [A], [B], [C])?

Aditya-138-12 · 2025-02-11T08:05:36Z

Hi @hageboeck,
Right now the f1.SetParameter(5, -1.234) f1.GetParameter(5) are working as expected.
But the f1.SetParameter("A", -1.234) f1.GetParameter("A") is not working and producing this error Error in <TFormula::GetParameter>: Parameter A is not defined. nan

Could you please check once where I am going wrong with this? A small direction would be really helpful in resolving the issue.
Thank you for your time and support!!

hageboeck · 2025-02-11T16:57:18Z

hist/hist/src/TFormula.cxx

+                  TString paramStr = static_cast<TObjString*>(tokens->At(1))->GetString();
+                  paramStr.Strip(TString::kBoth);
+                  // Support for parameter placeholders like [A]
+                  if (paramStr.BeginsWith("[") && paramStr.EndsWith("]")) {
+                     paramStr = paramStr(1, paramStr.Length()-2);
+                  }
+                  counter = paramStr.Atoi();


This looks to be the code where a named parameter is parsed. It seems to try to convert something like A into an integer in the last line, but I would expect it to first register this parameter in some kind of list of named parameters, and to obtain an index for that parameter.
Maybe this is the answer why pol(x, [A]) and similar doesn't work.

Aditya-138-12 requested review from bellenot and lmoneta as code owners February 5, 2025 11:59

dpiparo requested a review from hageboeck February 6, 2025 08:25

dpiparo assigned lmoneta Feb 6, 2025

hageboeck reviewed Feb 6, 2025

View reviewed changes

Removed BuildPolynomial Function Extended Syntax Support,

6757c9a

Aditya-138-12 requested a review from hageboeck February 10, 2025 08:52

hageboeck reviewed Feb 10, 2025

View reviewed changes

hageboeck reviewed Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TFormula] TFormula: Pol functions do not accept variable name as arguments #17623

[TFormula] TFormula: Pol functions do not accept variable name as arguments #17623

Aditya-138-12 commented Feb 5, 2025

hageboeck left a comment

hageboeck Feb 6, 2025 •

edited

Loading

Aditya-138-12 Feb 6, 2025 •

edited

Loading

hageboeck Feb 6, 2025

hageboeck Feb 6, 2025

Aditya-138-12 Feb 6, 2025 •

edited

Loading

hageboeck Feb 6, 2025

hageboeck Feb 6, 2025

hageboeck Feb 6, 2025

Aditya-138-12 Feb 6, 2025 •

edited

Loading

hageboeck Feb 6, 2025

Aditya-138-12 Feb 6, 2025

hageboeck left a comment

hageboeck Feb 10, 2025

hageboeck Feb 10, 2025

Aditya-138-12 Feb 10, 2025

hageboeck Feb 10, 2025

Aditya-138-12 commented Feb 11, 2025

hageboeck Feb 11, 2025

	////////////////////////////////////////////////////////////////////////////////
	/// Create a uniform binning under name 'name' for this variable.
	/// \param[in] nBins Number of bins. The limits are taken from the currently set limits.
	/// \param[in] name Optional name. If name is null, install as default binning.
	void RooRealVar::setBins(Int_t nBins, const char* name) {
	setBinning(RooUniformBinning(getMin(name),getMax(name),nBins),name);
	}

		/// Extended Syntax: polN(var, M) - Polynomial of degree N with variable var and parameters starting at [M]
		/// polN(var, [A]) - Polynomial of degree N with variable var and parameters starting at [A]

		TFormula f1("f1", "pol1(x,[5])");
		EXPECT_EQ(f1.GetExpFormula(), TString("([p5]+[p6]*x)"));

-    TFormula f1("f1", "pol1(x,[5])");
-    EXPECT_EQ(f1.GetExpFormula(), TString("([p5]+[p6]*x)"));
+    TFormula f1("f1", "pol1(x,[A])");
+    EXPECT_EQ(f1.GetExpFormula(), TString("([p5]+[p6]*x)"));
+    f1.SetParameter("A", -1.234);
+    f1.SetParameter(6, -1.235);
+    EXPECT_EQ(f1.GetParameter("A"), -1.234);
+    EXPECT_EQ(f1.GetParameter(5), -1.234);
+    EXPECT_EQ(f1.GetParameter(6), -1.235);
+    f1.SetParameter(5, -1.236);
+    EXPECT_EQ(f1.GetParameter("A"), -1.236);
+    EXPECT_EQ(f1.GetParameter(5), -1.236);
+    EXPECT_EQ(f1.GetParameter(6), -1.235);

[TFormula] TFormula: Pol functions do not accept variable name as arguments #17623

Are you sure you want to change the base?

[TFormula] TFormula: Pol functions do not accept variable name as arguments #17623

Conversation

Aditya-138-12 commented Feb 5, 2025

This Pull request:

Changes or fixes:

Checklist:

hageboeck left a comment

Choose a reason for hiding this comment

hageboeck Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Aditya-138-12 Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aditya-138-12 Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aditya-138-12 Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hageboeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aditya-138-12 commented Feb 11, 2025

Choose a reason for hiding this comment

hageboeck Feb 6, 2025 •

edited

Loading

Aditya-138-12 Feb 6, 2025 •

edited

Loading

Aditya-138-12 Feb 6, 2025 •

edited

Loading

Aditya-138-12 Feb 6, 2025 •

edited

Loading