feat: Make Posterior Predictives Work With 2D Binnings #654
Conversation
|
Hi @BenJarg, thank you for contributing to MaCh3! Please wait for MaCh3 developers to review your PR. If no one answers within a week, please message people from this list: https://github.com/orgs/mach3-software/teams/mach3admin . While waiting, please enjoy this  Use this action on your projects. Use jokes on issues instead. |
BenJarg
changed the title
KSkwarczynski
left a comment
KSkwarczynski
left a comment
There was a problem hiding this comment.
I would also add that on T2K-FD we do pvalue based on 1D projection (I don't rmeber why now) so wonder if it is possible to add setting which enforces to use 1D even for 2D samples.
Also CI is failing because of
https://github.com/mach3-software/MaCh3Tutorial/blob/a1e1726d86eb63919111d3e22c72419d9276f2cb/CIValidations/PredictiveValidations.cpp#L77C1-L88C6
| auto* h2 = dynamic_cast<TH2*>(Toys[sample][0].get()); | ||
| if (dim == 0) | ||
| refHist = h2->ProjectionX(); | ||
| else | ||
| refHist = h2->ProjectionY(); | ||
| } | ||
| else { | ||
| MACH3LOG_ERROR("Asking for {} with N Dimension = {}. This is not implemented", __func__, nDims); | ||
| throw MaCh3Exception(__FILE__, __LINE__); | ||
| } |
There was a problem hiding this comment.
Is ProjectionX/Y thread safe. I usually find any operation with ROOT making "new" histgoram not being thread safe :(
There was a problem hiding this comment.
Looking into this, as long as two threads aren't touching the same TH at once, it should be ok, so I think it is. The only parallel parts are serial over the dimensions direction.
There was a problem hiding this comment.
I am gonna use Clone as example as I had many trouble with it.
When you call Clone root is creating new object, but it is using "buffer" which has some operation on directory.
So if you start calling Clone in parallel loop, even if each thread is cloning different TH I still encountered plenty of issues.
And maybe it is fine with ProjectionX as I don't have as much experience with this one
There was a problem hiding this comment.
For example Projection calls gROOT->FindObject
https://github.com/root-project/root/blob/56be091a18cc07bc12242d65447d63611b4ebda4/hist/hist/src/TH2.cxx#L2195
I am not saying this isn't thread safe, but this looks like an operation going very deep into ROOT directory operations.
Fitters/PredictiveThrower.cpp
Outdated
| std::vector<std::vector<double>> MaxValue(TotalNumberOfSamples); | ||
| #ifdef MULTITHREAD | ||
| #pragma omp parallel for collapse(2) | ||
| #pragma omp parallel for |
There was a problem hiding this comment.
I don't think you need to make dimension for every dimension.
It should be doable to just have total bin number.
For 2D it would be yBin*nXbins + xBin
There was a problem hiding this comment.
Yeah good point. When writing it, I was under the impression M3 could already do binning in more than 2D, so I was trying to make it very general, but this is unnecessary for 2D.
There was a problem hiding this comment.
Looping back to this, I'm remembering different samples can have different dimensions, so it wouldn't be that simple. It could be done by looping through to check, but would that be more or less complicated than a 2D vector
There was a problem hiding this comment.
My motivation is that your solution will only work if bins are uniform (grid/TH2D) however it will not work with non-uniform grid (for example TH2Poly).
MaCh3 currently doesn't support non-unifrom but there are plans for this.
If you this this is too much work for now can you just add /// @todo SomeText
and we can sort this out later?
|
After todays MaCh3-T2K meeting I think you can ignore my comment of overwriting and using 1D for 2D |
BenJarg
force-pushed
the
feature/BenJarg/2DPosteriorPredictives
branch
from
21126f2 to
73d208c
Compare
Pull request description
Mostly just involves changing
TH1Ds ->TH1, which can be 1 or 2D. Two issues I had to deal with:ProduceSpectrauses violin plots, which expects 1D histograms. My workaround is I use the projections of a 2D histogram instead of the full thing. A lot of messy logic here, so comments are welcome. Also, I'm not sure if these projections are causing a memory leak.MakeFluctuatedHistogramAlternativefor TH2D's.