Skip to content

Conversation

@parth-07
Copy link
Contributor

@parth-07 parth-07 commented Apr 9, 2021

This PR aims to fix the problem of inconsistency in TFormula::SetParNames() and TFormula::SetParameters() functions as described in #7805.

I have added warning if more values are provided than the actual number of parameters of TFormula object, original behaviour of the function is maintained apart from the added warning.

Some test will fail due to the added warning, please tell if I should remove the warning or resolve the root cause of the warnings ( For instance, TF1 using TFormula::SetParameters() and always passing 11 arguments is one cause of failing tests)

I would also like to slightly modify behaviour of these functions such that value and names of only specified parameters should be changed, current behaviour is to reset the value and names of parameters which aren't specified, but this change may negatively impact programs using these functions. But still, I think this change will make these functions more intuitive. Please tell if I should add this change.

This PR fixes #7805

@lmoneta lmoneta self-assigned this Apr 12, 2021
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.

Very nice improvements and I agree it is consistent now.
I think we should then fixes also TF1::SetParNames and TF1::SetParameters to be consistent and since TF1 is probably more used than TFormula itself

@parth-07
Copy link
Contributor Author

Very nice improvements and I agree it is consistent now.
I think we should then fixes also TF1::SetParNames and TF1::SetParameters to be consistent and since TF1 is probably more used than TFormula itself

Yes agreed, I will make these changes in this PR.

@parth-07
Copy link
Contributor Author

parth-07 commented May 1, 2021

I think we should then fixes also TF1::SetParNames and TF1::SetParameters to be consistent and since TF1 is probably more used than TFormula itself

TF1::SetParNames and TF1::SetParameters member functions are virtual, and C++ currently do not support virtual function templates. Can you suggest some other way to make these functions generic ? Normal variadic functions can be used, but they are not very easy to work with and maintain, and also do not provide static type checking.

@lmoneta
Copy link
Member

lmoneta commented May 10, 2021

I see, I understand the issue with TF1::SetParameters. There is probably no need to have those functions virtual, in ROOT we do not have a derived class re-implementing those, but we cannot say this for user defined classes.
These improvements in TF1 could be then done later in a separate PR

@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from lmoneta Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from lmoneta Sep 28, 2023
@guitargeek guitargeek force-pushed the refactor-TFormula-parameters branch from b9758d5 to 73f26bc Compare September 28, 2023 20:09
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@guitargeek
Copy link
Contributor

Since I still think this is a nice improvement, I resurrected this PR by rebasing the changes on top of ROOT master. Let's see what the CI says now!

@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@guitargeek guitargeek force-pushed the refactor-TFormula-parameters branch 2 times, most recently from fa27846 to 751e434 Compare September 28, 2023 21:51
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@guitargeek guitargeek force-pushed the refactor-TFormula-parameters branch from 751e434 to 7a06e35 Compare September 28, 2023 21:52
@root-project root-project deleted a comment from phsft-bot Sep 28, 2023
@guitargeek
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link

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

@phsft-bot
Copy link

Build failed on mac11/noimt.
See console output.

@github-actions
Copy link

Test Results

         5 files           5 suites   1d 5h 46m 49s ⏱️
  2 476 tests   2 476 ✔️ 0 💤 0
11 647 runs  11 647 ✔️ 0 💤 0

Results for commit 7a06e35.

@guitargeek guitargeek changed the title Fix inconsistent behaviour of TFormula parameter functions [math] Fix inconsistent behaviour of TFormula parameter functions Sep 29, 2023
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Hi @parth-07! I have resurrected this PR in order to close the underlying issue.

I simplified your implementation a bit, and also did the same treatment with the variadic template methods to the TF1Parameters class.

I didn't do this for the TF1 though, because variadic templates would break the support for the context menu in the TBrowser. In addition, there is the problem that it's a virtual function as @lmoneta pointed out.

But what is consistent now is that both SetParNames and SetParameters have default argument values that correspond to the parameters or names not changing. For the Parameters, this is encoded by NaN, and for the names it is encoded with empty strings.

@guitargeek guitargeek merged commit fee2327 into root-project:master Oct 14, 2023
@parth-07
Copy link
Contributor Author

Hi @guitargeek, thank you for resurrecting the pull request.

maksgraczyk pushed a commit to maksgraczyk/root that referenced this pull request Jan 12, 2024
…ot-project#7812)

This commit aims to fix the problem of inconsistency in `TFormula::SetParNames()` and `TFormula::SetParameters()` functions as described in root-project#7805.

I have added warning if more values are provided than the actual number of parameters of `TFormula` object, original behaviour of the function is maintained apart from the added warning.

Some test will fail due to the added warning, please tell if I should remove the warning or resolve the root cause of the warnings ( For instance, `TF1` using `TFormula::SetParameters()` and always passing 11 arguments is one cause of failing tests) 

I would also like to slightly modify behaviour of these functions such that value and names of only specified parameters should be changed, current behaviour is to reset the value and names of parameters which aren't specified, but this change may negatively impact programs using these functions. But still, I think this change will make these functions more intuitive. Please tell if I should add this change. 

This commit fixes root-project#7805

---------

Co-authored-by: Jonas Rembser <jonas.rembser@cern.ch>
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.

Inconsistent and unintuitive behaviour of TFormula::SetParNames and TFormula::SetParameters

4 participants