DPL Analysis: Event mixing interface#7827
DPL Analysis: Event mixing interface#7827ktf merged 9 commits intoAliceO2Group:devAliceO2Group/AliceO2:devfrom
Conversation
|
Note: I left the MixedEventPartitionedTask, as I haven't tested partitions and filters thoroughly yet, after the recent filter changes. I will do it soon. Note 2: Hash task will be replaced by a dedicated struct / lambda, this will come in another PR. |
|
@saganatt it does not pass the CI in O2Physics. This is unexpected, right? |
There was a problem hiding this comment.
There is already functions which do something like this elsewhere. Check Framework/FunctionalHelpers.h.
There was a problem hiding this comment.
I am not sure this does what we want. Or, at least I don't see how this enables repeated calls of a function?
There was a problem hiding this comment.
Same here. Please, address the comment about pack_to_tuple and the we discuss again.
ktf
left a comment
There was a problem hiding this comment.
Thank you for the contribution! We need however a few changes before this can be merged. Could you have a look at the inline comments? It would also be good if you could split up the PR in smaller changes, so that things which are unambiguous can be merged immediately while less trivial changes have less noise around them.
|
Thanks @ktf , I corrected according to your suggestions, please see my comments. I am not sure about splitting - I can push Pack.h separately, but the rest depends on GroupedCombinations as a whole. So it is still the matter of completing grouped combinations. |
There was a problem hiding this comment.
Why not simply introducing a pack_to_tuple(pack<...>) helper, with the associated pack_to_tuple_t<pack<...>>? Then expressing the interleaved tuple should be just a matter of using those helpers, no? I think this part would become greatly simplified. Could you give it a try?
|
@jgrosseo see the other comment, I think the whole InterleavedTupleType can be simplified. While I appreciate this should go in asap, I really would like to try to reduce it to the minimum the usage of std::tuple, because in the past it was correlated with (even slower) compilation times. |
|
@ktf, I am trying to re-implement what you suggest, I will push it today/early tomorrow (sorry for the weekend time!). In general: I could replace tuple with a vector, for example. But not with a pack, I need something which holds the actual data. And I thought tuple is the recommended choice? Like for the usual combinations? |
|
Sure, you can keep the tuple, but to calculate an interleaved tuple you can simply interleave the associated pack and then transform the pack into a tuple declaration. If you can use an std::array (not a vector, which would allocate extra memory on the heap) it would be even better. |
59fdce3 to
dd6f618
Compare
|
I simplified the code and put pack-related functions again in Pack.h. |
dd6f618 to
4f10ab0
Compare
|
Sorry, I forgot to add the tests. All checks passed before, so should be still OK. Do you want a separate PR for pack changes? @ktf are we happy now with interleaveTuples() and functionToTuple() in GroupedCombinations.h? Let me know what needs fixing. And a perhaps trivial question to @jgrosseo : I quietly skip collision pairs (tuples) if any of the collisions does not contain any tracks. Is there a need to output to the user all collision pairs, even those with some track tables empty? |
| { | ||
| static_assert(sizeof...(T2s) > 0, "There must be associated tables in process() for a correct pair"); | ||
| static_assert(!soa::is_soa_iterator_t<std::decay_t<H>>::value, "Only full tables can be in process(), no grouping"); | ||
| if constexpr (std::conjunction_v<std::is_same<G, TG>, std::is_same<H, TH>>) { |
There was a problem hiding this comment.
For passing the tables to the manager? But then the original tables (collisions, tracks) accessible inside process() would be invalidated (ownership loss), no? Even if there is no use case for using these tables in an event mixing task, this would be surely confusing to users. Similarly, in PartitionManager, we pass tables just by &.
There was a problem hiding this comment.
No, I meant why not using std::is_same_v<G, TG> && std::is_same_v<H, TH>. We can fix it later.
|
I was actually thinking: template <typename T,int... Is>
inline auto sequence_to_pack(std::integer_sequence<int, Is...>){
return pack<decltype((Is,T{}))...>{};
};
template <typename T, std::size_t N>
using repeated_type_pack_t = decltype(sequence_to_pack<T>(std::make_index_sequence<N>()));might be a bit more orthogonal. |
|
Now I don't fully understand how |
|
Given: the comma operator evaluates all expression in order and then returns the value of the last expression. |
|
ah, right, thank you! |
| template <typename T1, typename GroupingPolicy, typename H, typename G, typename... Us, typename... As> | ||
| struct GroupedCombinationsGenerator<T1, GroupingPolicy, H, G, pack<Us...>, As...> { | ||
| using joinIterator = typename soa::Join<H, G>::table_t::iterator; | ||
| using GroupedIteratorType = pack_to_tuple_t<interleaved_pack_t<repeated_type_pack_t<joinIterator, sizeof...(As)>, pack<As...>>>; |
There was a problem hiding this comment.
repeated_type_pack_t<joinIterator, sizeof...(As)> is probably better written as:
pack<(decltype(As, joinIterator{}))...>
so that you avoid the repacking...
There was a problem hiding this comment.
yes, I can avoid repeated_type_pack_t.
I wonder also about the interleaved_pack_t now, but apparently it is too tricky to get two variadics expanding at once, and not just the second one....
There was a problem hiding this comment.
Yes, I wondered about the same, but again, let's merge this and then we can continue.
|
That said, let's avoid other changes for now and get the tests to finish. We can improve later. It's already quite an improvement. |
|
Tested on hyperloop, merging. It would be good to go through Framework/GroupedCombinations.h and see if some further simplifications cannot be made. |
Hi, this actually introduces the new event mixing. I will refresh the tutorial in O2Physics once this PR will get merged.
In the meantime, you can see the new interface here. It is possible to define it both outside and inside
process().