[GSoC] Hist and TGraph Pythonizations. #11357
Conversation
|
Can one of the admins verify this patch? |
|
@etejedor Sir, Here's a link to the github gist for your reference. |
|
Hello @Harshalzzzzzzz
What is exactly the problem? |
|
TGraph Constructor returns a null value |
bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tgraph.py
Outdated
Show resolved
Hide resolved
bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tgraph.py
Outdated
Show resolved
Hide resolved
Please see my comments inline in the code about this. |
|
@etejedor Can you also please review the GetAsNumpy method, so that I can resolve the errors |
What kind of errors do you get? |
@etejedor Thank you sir, both errors are resolved, Kindly review the PR if possible. |
|
@lmoneta @sanjibansg @omazapa Thanks. |
bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tgraph.py
Outdated
Show resolved
Hide resolved
bindings/pyroot/pythonizations/python/ROOT/_pythonization/_th1.py
Outdated
Show resolved
Hide resolved
| def _GetBin(self, *args): | ||
| a = _numpy_content(self._Original_GetBin(*args), *args) | ||
| return a | ||
|
|
||
|
|
||
| def _GetBinCenter(self, *args): | ||
| a = _numpy_content(self._Original_GetBinCenter(*args), *args) | ||
| return a | ||
|
|
||
|
|
||
| def _GetBinContent(self, *args): | ||
| a = _numpy_content(self._Original_GetBinContent(*args), *args) | ||
| return a | ||
|
|
||
|
|
||
| def _GetBinLowEdge(self, *args): | ||
| a = _numpy_content(self._Original_GetBinLowEdge(*args), *args) | ||
| return a |
There was a problem hiding this comment.
I don't see the point of these Pythonizations of GetBin, GetBinCenter, GetBinContent, and GetBinLowEdge. All the original functions return a scalar, and in this case the _numpy_context function just returns the scalar you pass to it.
Can you please elaborate?
There was a problem hiding this comment.
Same for the TH2 and TH3 functions
There was a problem hiding this comment.
The bin contents are stored in the fArray data member of the TH1 (here the header file for reference).
To implement GetBinContent in an efficient way for the full array, you would have access this fArray member and create a numpy array from it with frombuffer. However, I think this member is not public. Maybe we can add a public accessor to TH1, or what would you do here @lmoneta?
There was a problem hiding this comment.
For the bin centers and edges, it's easier. You can access the array of bin edges with public getters from the TAxis instances in the histogram, which you can for example get like hist.GetXaxis(). Then you got to study the TAxis class a bit to understand how to retrieve bin boundaries best in NumPy.
I guess if you have non, uniform binning, you can get the full bin edges array with hist.GetXaxis().GetXbins().GetArray(). For uniform bins, you can just create a np.linspace based on the limits and the number of bins
There was a problem hiding this comment.
@guitargeek Sir when I try to use something like hist.GetSumw2() to convert this I get segmentation error, when I try to return the bin sum of weight square while implementing GetContent()
Link to updated gist.
There was a problem hiding this comment.
I have a few technical suggestions, and a question about what these GetBinContent et al. pythonizations actually do (please clarify).
Also, I'm not not convinced by the approach to the FromNumpy function. If you want to support filling of histograms from NumPy arrays, why not Pythonize Fill() and FillN(), which is probably the first thing people will try out intuitively? And once that exists, I'm nut sure if a FromNumpy function will add much more value to the pythonizations, if most arguments are just forwarded to the constructor anyway, and the additional NumPy arguments could be passed to fill.
Something to discuss also with @lmoneta.
bindings/pyroot/pythonizations/python/ROOT/_pythonization/_th_utils.py
Outdated
Show resolved
Hide resolved
|
@guitargeek We have already |
|
Okay! Having |
|
This pull request introduces 4 alerts when merging cb774a4 into 2441d13 - view on LGTM.com new alerts:
|
|
This pull request introduces 4 alerts when merging 3285d60 into b7a3cb5 - view on LGTM.com new alerts:
|
|
This pull request introduces 4 alerts when merging 538d1cf into 97802b9 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging b5f4c78 into e7c7170 - view on LGTM.com new alerts:
|
This Pull request:
Changes or fixes:
Checklist:
This PR fixes #