[DPL Analysis] [Event mixing] Introducing Binning Policy for block combinations by saganatt · Pull Request #8299 · AliceO2Group/AliceO2

saganatt · 2022-03-09T13:41:45Z

Ciao @ktf,

The BinningPolicy classes replace the special hash task / table for block combinations.

Sorry for a huge PR, but I needed to modify in tests all lines that call block combinations, otherwise it won't compile.
The core changes are: new BinningPolicy.h and rewritten groupTable() in ASoAHelpers.h.

…r templated groupTable()

saganatt · 2022-03-09T13:45:14Z

Hi @jgrosseo,

in the next PR I will adjust event mixing interface. With the changes introduced here, event mixing will look like this:

std::vector<double> zBins{VARIABLE_WIDTH, -7.0, -5.0, -3.0, -1.0, 1.0, 3.0, 5.0, 7.0};
std::vector<double> yBins{VARIABLE_WIDTH, 0, 5, 10, 20, 30, 40, 50, 101};
PairBinningPolicy<aod::cent::CentEstV0M, aod::collision::PosZ> pairBinning{yBins, zBins};
SameKindPair<aod::Collisions, aod::Tracks> pair{pairBinning, 5, -1};
void process (soa::Join<aod::Collisions, aod::CentV0Ms>>& collisions, aodTracks const& tracks) {
    for (auto& [c1, tracks1, c2, tracks2] : pair) { ... }
}

"pair binning" means "calculate bins based on 2 columns". Similarly, there is SingleBinning and TripleBinning. We can think about a better naming, if it is confusing.

What do you think about this approach?

saganatt · 2022-03-09T13:46:37Z

Framework/Core/include/Framework/ASoAHelpers.h

+  auto binningColumns = binningPolicy.getColumns();
+  auto arrowColumns = o2::framework::binning_helpers::getArrowColumns(arrowTable, binningColumns);
+  auto chunksCount = arrowColumns[0]->num_chunks();
+  // TODO: Are such checks needed or can we safely assume chunks are always the same?


@ktf?
If I don't need to check the alignments, then I can get rid of getArrowColumns() and getChunks() altogether.

For sure arrow arrows mixing chunks sizes / number of chunks. I would not assume they are necessarily the same (although it might be in our case so far).

saganatt · 2022-03-09T13:51:47Z

Framework/Core/include/Framework/BinningPolicy.h

+template <typename... Cs>
+std::array<arrow::ChunkedArray*, sizeof...(Cs)> getArrowColumns(arrow::Table* table, pack<Cs...>)
+{
+  static_assert(std::conjunction_v<typename Cs::persistent...>, "BinningPolicy: only persistent columns accepted (not dynamic and not index ones");


An important point: one cannot do event mixing / block combinations based on values in dynamic columns. Simply because they do not exist as real columns.

jgrosseo · 2022-03-10T07:53:47Z

Thanks for this work! The interface looks good to me. I let @ktf comment on the technicalities.

About the naming. So we cannot have just BinningPolicy with a different number of template arguments? Then maybe simply BinningPolicy1 BinningPolicy2 ... ?

jgrosseo · 2022-03-10T07:54:23Z

It breaks the compilation of O2Physics so we would need to force merge both in parallel. Is there an easy trick to avoid this?

saganatt · 2022-03-10T08:21:42Z

About the naming. So we cannot have just BinningPolicy with a different number of template arguments? Then maybe simply BinningPolicy1 BinningPolicy2 ... ?

BinningPolicyBase has a variable number of template arguments. I created Single, Pair, Triple policies so as to override getBin(). Perhaps this function could be generalized to any number of input columns, but I would need more time to think how.

On the other hand, are there many (any?) use cases of mixing based on 4 or more event properties?

jgrosseo · 2022-03-10T08:34:24Z

On the other hand, are there many (any?) use cases of mixing based on 4 or more event properties?

No, 1-3 is sufficient.

saganatt · 2022-03-10T08:35:17Z

It breaks the compilation of O2Physics so we would need to force merge both in parallel. Is there an easy trick to avoid this?

Ah, right. I changed the selfCombinations parameters. I can write a temporary binning class for backward compatibility but perhaps this is not worthy the effort? I see it is used in 4 places, in connection with event mixing. With my next PR that will need to change anyways.

saganatt · 2022-03-11T14:42:14Z

I added the backward compability.

About the naming. So we cannot have just BinningPolicy with a different number of template arguments? Then maybe simply BinningPolicy1 BinningPolicy2 ... ?

I implemented a single binning class. But I am curious whether this code looks good (wrt performance) to @ktf ? Or whether old implementation with multiple classes was better?

I also commented lines that caused the problem in macOS, as I am not sure we will need them at the end (see my review comments above, ASoAHelpers.h:88).

ktf · 2022-03-15T07:54:14Z

Framework/Core/include/Framework/ASoAHelpers.h

+{
+template <typename T>
+struct is_string {
+  static const bool value = false;
+};
+
+template <class T, class Traits, class Alloc>
+struct is_string<std::basic_string<T, Traits, Alloc>> {
+  static const bool value = true;
+};
+
+template <std::size_t N>
+struct is_string<char[N]> {
+  static const bool value = true;
+};
+} // namespace old_interface
+


I think you could just use is_specialization, no?

ktf · 2022-03-15T07:54:43Z

Framework/Core/include/Framework/ASoAHelpers.h


 #include "Framework/ASoA.h"
+#include "Framework/BinningPolicy.h"
+#include "Framework/Logger.h"


Please, no Logger.h here.

ktf · 2022-03-15T07:55:18Z

Framework/Core/include/Framework/ASoAHelpers.h

+  uint64_t ind = 0;
+  uint64_t selInd = 0;
+  gsl::span<int64_t const> selectedRows;
+  std::vector<std::pair<uint64_t, uint64_t>> groupedIndices;


I would use a struct rather than a pair.

ktf

I am afraid the changes are too large for me to review them thoroughly, but the idea looks fine, apart from a few nitpicks that can be fixed later (please make sure you drop Logger.h, in particular). Assuming you tested them and in particular you made sure grouping still works, I am fine with them.

saganatt added 6 commits March 7, 2022 15:09

Current progress - almost everything for block combinations except fo…

0a394b4

…r templated groupTable()

Fully working binning policy, tests adapted

760ea48

Added tests for triple binning

71db432

Cleaner code in binning policy

6aa9b7f

Move helper functions out of the binning class

e9658c8

Final fixes and cleanup

522038a

saganatt requested a review from a team as a code owner March 9, 2022 13:41

saganatt commented Mar 9, 2022

View reviewed changes

A few nitpicks with Binning Policy in templates in combinations

e1047cd

Fix expression result unused error on macOS

7ec00a3

saganatt added 4 commits March 10, 2022 10:38

Fix for_ on macOS

dd38a39

Add backward compability to build O2Physics

44e9630

Comment out the part that causes error in MacOS

40f87a0

Unify binning policies into one class

7559036

ktf reviewed Mar 15, 2022

View reviewed changes

ktf approved these changes Mar 15, 2022

View reviewed changes

ktf merged commit 9f30bb8 into AliceO2Group:dev Mar 15, 2022

saganatt mentioned this pull request Mar 15, 2022

[DPL Analysis] [Event mixing]: Follow-up to #8299 #8362

Merged

ktf pushed a commit that referenced this pull request Mar 17, 2022

[DPL Analysis] [Event mixing]: Follow-up to #8299 (#8362)

8e3996a

Conversation

saganatt commented Mar 9, 2022

Uh oh!

saganatt commented Mar 9, 2022

Uh oh!

saganatt Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

ktf Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

saganatt Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

jgrosseo commented Mar 10, 2022

Uh oh!

jgrosseo commented Mar 10, 2022

Uh oh!

saganatt commented Mar 10, 2022

Uh oh!

jgrosseo commented Mar 10, 2022

Uh oh!

saganatt commented Mar 10, 2022

Uh oh!

saganatt commented Mar 11, 2022

Uh oh!

ktf Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

ktf Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

ktf Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

ktf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants