[math][mathmore] Fix setting optional parameters for GSLMCIntegrator#14058
Conversation
|
Starting build on |
Test Results 9 files 9 suites 1d 16h 37m 51s ⏱️ Results for commit ebb2113. ♻️ This comment has been updated with latest results. |
| auto extraOpts = ExtraOptions(); | ||
| ROOT::Math::IntegratorMultiDimOptions opt(extraOpts.release()); |
There was a problem hiding this comment.
| auto extraOpts = ExtraOptions(); | |
| ROOT::Math::IntegratorMultiDimOptions opt(extraOpts.release()); | |
| ROOT::Math::IntegratorMultiDimOptions opt(ExtraOptions().release()); |
Can be shortened (which also makes clear that the output of ExtraOptions(I) is not used anywhere else)
| opt->SetIntValue("mode",mode); | ||
| opt->SetIntValue("verbose",verbose); | ||
| return opt; | ||
| return std::unique_ptr<ROOT::Math::IOptions>(opt); |
There was a problem hiding this comment.
| return std::unique_ptr<ROOT::Math::IOptions>(opt); | |
| return opt; |
To really avoid manual memory management with operator new, it would be better to directly create opt as a unique_ptr and then simply return it.
And then further up, this would mean doing:
auto opt = std::make_unique<GenAlgoOptions>();
| opt->SetIntValue("min_calls",min_calls); | ||
| opt->SetIntValue("min_calls_per_bisection",min_calls_per_bisection); | ||
| return opt; | ||
| return std::unique_ptr<ROOT::Math::IOptions>(opt); |
There was a problem hiding this comment.
| return std::unique_ptr<ROOT::Math::IOptions>(opt); | |
| auto opt = std::make_unique<GenAlgoOptions>(); | |
| .... | |
| return out; |
Same as in my other comment.
|
|
||
| /// convert to options (return object is managed by the user) | ||
| ROOT::Math::IOptions * operator() () const; | ||
| std::unique_ptr<ROOT::Math::IOptions> operator() () const; |
There was a problem hiding this comment.
| std::unique_ptr<ROOT::Math::IOptions> operator() () const; | |
| std::unique_ptr<ROOT::Math::IOptions> makeIOptions() const; |
Since you are already changing the return type of that function, can we maybe also change the name?
Personally, I don't like implementing operator() if there is not a really good reason for it, because if is harder to grep the code for where it is actually called. It's better to have an explicit name I think.
There was a problem hiding this comment.
Also, since this returns now a unique_ptr, the comment "is managed by the user" is not necessary anymore:
Convert to options.Further note that doxygen comments should begin uppercase and end with a period.
|
|
||
| /// convert to options (return object is managed by the user) | ||
| ROOT::Math::IOptions * operator() () const; | ||
| std::unique_ptr<ROOT::Math::IOptions> operator() () const; |
There was a problem hiding this comment.
| std::unique_ptr<ROOT::Math::IOptions> operator() () const; | |
| std::unique_ptr<ROOT::Math::IOptions> makeIOptions() const; |
Same here.
| std::unique_ptr<ROOT::Math::IOptions> ExtraOptions() const; | ||
|
|
||
| /** | ||
| * set the extra options for Vegas and Miser |
There was a problem hiding this comment.
| * set the extra options for Vegas and Miser | |
| * Set the extra options for Vegas and Miser. |
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
LGTM in principle! I just have a few technical suggestions that would be nice to see implemented.
Calling GSLMCIntegrator::SetMode crashed because the internal workspace wss not yet created. This is now fixed and also the way the extra option parameters are retrieved and set. Avoid now potential memory leak by using a unique_ptr
use MakeIOptions() instead of operator()
62ada10 to
ebb2113
Compare
|
Starting build on |
3 participants
Calling
GSLMCIntegrator::SetModecrashed because the internal workspace was not yet created.This PR fixes this and also the way the extra option parameters are retrieved and set in the GSLMCIntegrator.
A small change in the interface is done by returning now a unique_ptr instead of a row pointer in
GSLMCIntegrator::ExtraOptions()