[v630][RF] Backports of RooFit PRs to v6-30-00-patches: Part 8
#14252
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes a small bug in calculation of product of diagonal terms in LU matrix decompositions. A unit test to cover the former bug is also implemented.
Instead of hard-coding 'lib' as the path to which minuit2 is installed
as a standalone library, use the user configurable CMAKE_INSTALL_LIBDIR.
As a particularly common example, this allows a user to specify the
library installation path to '${_prefix}/lib64' on 64-bit machines from
the cmake command line.
|
Starting build on |
23 tasks
In the current implementation, shapesys with no valid data (all constraints constant) are written out in an invalid way, making it impossible for the reader to then instantiate the correct number of parameters. This PR fixes this by forcing the write-out of data with all-zeros for such invalid shapesys. Alternatively, one could imagine dropping these shapesys completely, but that's maybe something of a policy decision that I don't want to make in a bugfix patch :)
Fixing a crash in HistoToWorkspaceFactoryFast.cxx where a parameter that was globally set to be constant was not found a given region, but the code was still accessing the parameter even when it was nullptr. Now the parameter is set to constant only when found in a given region. Also demoting the error to warning as this does not always indicate a wrong setup.
|
Starting build on |
|
Not sure if you want to catch in one of your PR also some stuff from the milestones: https://github.com/root-project/root/milestones/ |
|
Starting build on |
At some point in `RooGenProdProj::createIntegral()`, an intermediate integral object that should only live during the scope of the function is accidentally put in the `saveSet` output parameter. This needs to be fixed. Thanks to the following forum post for noticing this: https://root-forum.cern.ch/t/error-inputarguments-rooargset-error-argument-with-name-is-already-in-this-set-in-roomcstudy/57571
When compiling with gcc 13.2, there is a new warning in TMVA that is
fixed by this commit:
```c++
In static member function ‘static constexpr _Up* std::__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m(_Tp*, _Tp*, _Up*) [with _Tp = long unsigned int; _Up = long unsigned int; bool _IsMove = true]’,
inlined from ‘constexpr _OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = true; _II = long unsigned int*; _OI = long unsigned int*]’ at /usr/include/c++/13.2.1/bits/stl_algobase.h:506:30,
inlined from ‘constexpr _OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = true; _II = long unsigned int*; _OI = long unsigned int*]’ at /usr/include/c++/13.2.1/bits/stl_algobase.h:533:42,
inlined from ‘constexpr _OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = true; _II = long unsigned int*; _OI = long unsigned int*]’ at /usr/include/c++/13.2.1/bits/stl_algobase.h:540:31,
inlined from ‘constexpr _OI std::copy(_II, _II, _OI) [with _II = move_iterator<long unsigned int*>; _OI = long unsigned int*]’ at /usr/include/c++/13.2.1/bits/stl_algobase.h:633:7,
inlined from ‘static _ForwardIterator std::__uninitialized_copy<true>::__uninit_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = std::move_iterator<long unsigned int*>; _ForwardIterator = long unsigned int*]’ at /usr/include/c++/13.2.1/bits/stl_uninitialized.h:147:27,
inlined from ‘_ForwardIterator std::uninitialized_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = move_iterator<long unsigned int*>; _ForwardIterator = long unsigned int*]’ at /usr/include/c++/13.2.1/bits/stl_uninitialized.h:185:15,
inlined from ‘constexpr _ForwardIterator std::__uninitialized_copy_a(_InputIterator, _InputIterator, _ForwardIterator, allocator<_Tp>&) [with _InputIterator = move_iterator<long unsigned int*>; _ForwardIterator = long unsigned int*; _Tp = long unsigned int]’ at /usr/include/c++/13.2.1/bits/stl_uninitialized.h:373:37,
inlined from ‘constexpr _ForwardIterator std::__uninitialized_move_if_noexcept_a(_InputIterator, _InputIterator, _ForwardIterator, _Allocator&) [with _InputIterator = long unsigned int*; _ForwardIterator = long unsigned int*; _Allocator = allocator<long unsigned int>]’ at /usr/include/c++/13.2.1/bits/stl_uninitialized.h:399:2,
inlined from ‘constexpr void std::vector<_Tp, _Alloc>::_M_range_insert(iterator, _ForwardIterator, _ForwardIterator, std::forward_iterator_tag) [with _ForwardIterator = const long unsigned int*; _Tp = long unsigned int; _Alloc = std::allocator<long unsigned int>]’ at /usr/include/c++/13.2.1/bits/vector.tcc:819:9,
inlined from ‘constexpr std::vector<_Tp, _Alloc>::iterator std::vector<_Tp, _Alloc>::insert(const_iterator, std::initializer_list<_Tp>) [with _Tp = long unsigned int; _Alloc = std::allocator<long unsigned int>]’ at /usr/include/c++/13.2.1/bits/stl_vector.h:1411:17,
inlined from ‘void TMVA::MethodDL::ParseInputLayout()’ at /home/rembserj/spaces/master/root/src/root/tmva/tmva/src/MethodDL.cxx:470:24:
/usr/include/c++/13.2.1/bits/stl_algobase.h:437:30: error: ‘void* __builtin_memmove(void*, const void*, long unsigned int)’ forming offset 32 is out of the bounds [0, 32] [-Werror=array-bounds=]
437 | __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
|
```
The fix suggested in this commit is to directly assign from a full
initializer list, which is also more readable in the code.
In the RooFit `json_interface`, the calls to the actual JSON library are abstracted away, such that the libraries like `nlohmann_json` are not included in the headers. Therefore, `nlohmann_json` is a private dependency of the RooFit JSON interface, not a public one.
Using `nlohmann_json` as a public dependency of ROOT can result in different troubles, like root-project#14188. That's why it's better to avoid this dependency if we can, also to minimize the dependency of RooFit on the interface level. In the case of RooFit, the only reason for this dependency was the `HeatmapAnalyzer::getMetadata()` function. However, it just returned a json with a vector of string lables. We can also return a `std::vector<std::string>` here. I already talked with @Zeff020 about this change, and he is completely fine with it. The class was also only used by the RooFit Multiprocess developers so far, so changing the interface is fine. With this interface change, only some refactoring was necessary to avoid including `nlohmann_json` in the RooFit headers. This commit is similar to 9d7aa4a.
|
Starting build on |
Looks like these already got backported now. I just backport my own commits in these PRs, plus some others if I'm aware of them. But in the end, it's our shared responsibility to make sure that these backports are all landing 🙂 |
guitargeek
merged commit 793371f
into
root-project:v6-30-00-patches
9 of 14 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a backport of some RooFit PRs that were recently merged to master to v6-30-00-patches:
RooGenProdProj#14253nlohmann_jsonas a public dependency of RooFit #14266Related to #13458.
Link to the previous backport PR:
#14167