[df] Deprecate confusing signatures of HistoN[Sparse]D #21191
Conversation
The signatures of HistoN[Sparse]D operations allow passing the column that will provide the weights as the last element in the list of input columns, practically allowing for a size which is one more than the number of dimensions of the histogram to be filled. This was done to align these signatures with the logic of THnBase::Fill that follows the same schema. But these signatures are also different than all other Histo* signatures that accept weight columns as separate arguments. Thus, they are aligned to the other signatures by adding a new optional argument for the name of the column that will provide the weights. The previous usage of the operations is deprecated with a warning. Add tests.
Test Results 22 files 22 suites 3d 19h 14m 4s ⏱️ Results for commit ed3661d. |
| /// \param[in] columnList | ||
| /// A list containing the names of the columns that will be passed when calling `Fill`. | ||
| /// (N columns for unweighted filling, or N+1 columns for weighted filling) | ||
| /// \param[in] wName The name of the column that will provide the weights. |
There was a problem hiding this comment.
| /// \param[in] wName The name of the column that will provide the weights. | |
| /// \param[in] wName The name of the column that will provide the weights. | |
| /// \deprecated Passing the column with the weights as the last column in the list is deprecated. | |
| /// Instead, pass it as a separate argument, e.g. `HistoND(model, cols, weightCol)`. |
same suggestions below
There was a problem hiding this comment.
Also: potentially add mention in ReleaseNotes
dpiparo
left a comment
dpiparo
left a comment
There was a problem hiding this comment.
thanks for quickly implementing what decided at the meeting last week.
|
A candidate for |
I'm not opposed to this per se. But I wanted to highlight that this would set a precedent that we need to be ready to confirm also for other similar situations in the future. |
|
My two cents: If we are deprecating method overloads that have been there for a long time, this should be part of ROOT 6.40. My previous push to have this in ROOT 6.38.04 was based on the understanding that we want to remove an interface that was only introduced recently, which would be done better sooner than later. |
I agree. Since the two methods are similar, I would propose to have both HistoND and HistoNSparseD deprecated in 6.40, would you agree? |
4 participants
The signatures of HistoN[Sparse]D operations allow passing the column that will provide the weights as the last element in the list of input columns, practically allowing for a size which is one more than the number of dimensions of the histogram to be filled. This was done to align these signatures with the logic of THnBase::Fill that follows the same schema.
But these signatures are also different than all other Histo* signatures that accept weight columns as separate arguments. Thus, they are aligned to the other signatures by adding a new optional argument for the name of the column that will provide the weights.
The previous usage of the operations is deprecated with a warning.
Add tests.
Fixes #20816