[python][UHI] Implement Slicing in TH1 and adapt the UHI backend and tests #18735
Conversation
siliataider
force-pushed
the
UHI-indexing
branch
2 times, most recently
from
8f118cc to
9e29340
Compare
hist/hist/inc/TH1.h
Outdated
| auto setSliceContents = [&](auto... binEdges) { | ||
| ROOT::Internal::SetSliceContent(*hSlice, sliceContents, binEdges...); | ||
| }; | ||
| caller(setSliceContents, shiftedBins); |
There was a problem hiding this comment.
I think this whole arrangement with variadic templated arguments is really hard to understand and I'm wondering if it can be simplified on the C++ side (at the cost of some increased complexity in the pythonization).
As far as I understand, args are always expected to be int, so this function should probably take a vector or a span thereof rather than being variadic.
Also, since this is quite a long function I would add a doc comment explaining what it does.
hist/hist/inc/TH1.h
Outdated
| } | ||
|
|
||
| // Create a new histogram of the same type as this | ||
| std::unique_ptr<TH1> hSlice(static_cast<TH1*>(IsA()->GetNew()(nullptr))); |
There was a problem hiding this comment.
What does GetNew()(nullptr) do? I guess it calls the default constructor of the class? Maybe a note would be useful, since unfortunately the typedef of NewFunc_t doesn't bother explaining...
hist/hist/inc/TH1.h
Outdated
| : hSlice->GetBin(fXaxis.GetNbins() + 1, fYaxis.GetNbins() + 1, fZaxis.GetNbins() + 1); | ||
| hSlice->SetBinContent(overflowBin, overflowContent); | ||
|
|
||
| return hSlice.release(); |
There was a problem hiding this comment.
Why are we releasing the pointer here? I get this is meant to be called by Python, but if someone calls this from C++ and doesn't manually delete the histogram they would be leaking it silently.
Would it maybe be better to return the unique_ptr and then call SetOwner from python explicitly?
Test Results 18 files 18 suites 3d 20h 26m 55s ⏱️ For more details on these failures, see this check. Results for commit 33d1f1a. ♻️ This comment has been updated with latest results. |
| template <size_t Dim> | ||
| void SetSliceContent(const std::variant<std::vector<Double_t>, Double_t> &input, const std::vector<Int_t>& args) { |
There was a problem hiding this comment.
Maybe also the Dim template argument here could be just converted to an integer parameter to the function?
hist/hist/src/TH1.cxx
Outdated
| } | ||
|
|
||
| // Create a new histogram of the same type as this | ||
| std::unique_ptr<TH1> hSlice(static_cast<TH1*>(IsA()->GetNew()(nullptr))); |
There was a problem hiding this comment.
I would just call a function like
template <typename T>
std::unique_ptr<TH1> createHistoDefault(const T &histo){
TDirectory::TContext ctxt{nullptr};
return std::make_unique<T>();
}There was a problem hiding this comment.
Note that creating the new histogram without attaching it to gDirectory is important
hist/hist/src/TH1.cxx
Outdated
| : hSlice->GetBin(fXaxis.GetNbins() + 1, fYaxis.GetNbins() + 1, fZaxis.GetNbins() + 1); | ||
| hSlice->SetBinContent(overflowBin, overflowContent); | ||
|
|
||
| return hSlice.release(); |
There was a problem hiding this comment.
I also don't understand why we need to release the pointer here, can't we just return a unique_ptr?
| friend auto ROOT::Internal::Slice(const T &histo, std::vector<Int_t>& args); | ||
|
|
||
| template <size_t Dim> | ||
| void SetSliceContent(const std::variant<std::vector<Double_t>, Double_t> &input, const std::vector<Int_t>& args) { |
There was a problem hiding this comment.
Can you comment on the need for the std::variant parameter? It is not immediately clear to me, sorry.
There was a problem hiding this comment.
This is to either set the slice with a vector or a scalar,, in principle I could only keep the vector parameter and then construct the std vector from the scalar on the python side...
This Pull request:
Changes or fixes:
Previously, slicing was implemented at the pythonization level: when slicing a histogram, the values within the specified slice were kept and the ones outside were set to 0, without resizing the sliced axis (leading to inconsistencies..).
This PR implements the slicing logic in
TH1directly, adapting the pythonizations to make use of the new internal functions.Other changes are made to enhance the interface and fully comply with the UHI specifications, improvements can be summarized here:
h[0:2, 0:2] = np.array([[42], [3]])).Checklist: