pctpairprotons in Python #5
Conversation
CMakeLists.txt
Outdated
| TARGET_LINK_LIBRARIES(pctpaircuts ${ROOT_LIBRARIES} RooFit RooFitCore Minuit) | ||
| ADD_PCT_EXECUTABLE(pctlluconverter pctlluconverter.cxx) | ||
| TARGET_LINK_LIBRARIES(pctlluconverter ${ROOT_LIBRARIES}) | ||
| ADD_PCT_EXECUTABLE(pctpaircuts pctpaircuts.cxx) |
There was a problem hiding this comment.
It's good to remove the root dependency but we can't entirely disable pctpaircuts. Would be nice to have a compilation define (e.g., PCT_USE_ROOT) to disable certain features in pctpaircuts.
There was a problem hiding this comment.
I have implemented such idea atop master here: acoussat@5ae18f5. Should I open a new PR with this code or update this PR?
There was a problem hiding this comment.
No, I think it's part of the same PR.
Nothing wrong with your code but I'd consider creating a pctConfiguration.h.in similar to RTK's rtkConfiguration.h.in. What do you think?
There was a problem hiding this comment.
Thanks for the comment! I have modified my code to mimic how RTK operates, and created pctConfiguration.h in commit 85074eb. Let me know what you think.
|
Following our discussion, 48fd2a5 removes the dependency on |
acoussat
force-pushed
the
pctpairprotons-python
branch
from
1529a94 to
c8f54a8
Compare
|
I just noticed that I did not re-implemented the optional sixth column with nuclear information. Should I also re-implement it or is it unnecessary at this stage? |
|
Yes, please reimplement it if it's not a lot of work. We can consider a reimplementation later on but it would be nice to start from just a translation of the original tool. Thanks in advance! |
acoussat
force-pushed
the
pctpairprotons-python
branch
from
9a0771d to
ec21a3d
Compare
Unfortunately, it's not as easy as it looks, as GATE 10 does not export branches |
This commit introduces three changes: - Provide the GATE simulation in GATE 10, and delete the old GATE 6 one - Delete C++ version of pctpairprotons, which is now superseded by the Python version - Change pctpairprotons.py to remove parameters that allowed to reorganize the GATE simulation output. pctpairprotons.py should now be easier to understand. A single commit was made for these three changes because the changes they introduce are somewhat interleaved.
acoussat
force-pushed
the
pctpairprotons-python
branch
from
8f18b94 to
293416e
Compare
Reimplementation of
pctpairprotonsin Python in order to remove the dependency on ROOT. The current behavior is that the script is always copied to the build folder, and the C++ version ofpctpairprotonsis only compiled whenPCT_WITH_ROOTis on. I checked on simple cases that the C++ and the Python versions produce the same outputs at the byte level.The Python version adds a dependency on
pandasto efficiently merge the two ROOT files. I couldn't find an easy way to reproduce the same behavior in pure NumPy. I did write a pure NumPy implementation of the same script, but it usesforloops pretty much like the C++ version does. I didn't include it in this PR, but it is accessible there: https://github.com/acoussat/PCT/blob/pctpairprotons-numpy/pctpairprotons_numpy.py. I didn't extensively compare the two implementations to check which one is faster, but I would guess thatpandas' optimized routine is probably faster.Let me know what you think!