Feature #22628
Change internal representation of sim::OnePhoton
100%
Description
sim::OnePhoton is part of one of the two supported representation of scintillation light in LArSoft.
It is in fact part of the most expensive one, where each scintillation photon is saved individually as an element of a sim::SimPhotons
collection.
Given that this is so expensive, it may be good to refresh and compact it.
The actions I propose are:
- replacement of
TVector3
withgeo::Point_t
in the two vector members - move of the
bool
data member at the end of the object
A survey in the code shows that the fixes after the first change are trivial.
Other actions that should take place at the same time:
- ordering operator (ordering by increasing
Time
, then byMotherTrackID
,InitialPosition
(x, y, z) andFinalLocalPosition
(x, y, z) - transformation into
struct
- removal of the empty constructor and explicit initialization of the data members
Associated revisions
History
#1 Updated by Kyle Knoepfel over 1 year ago
- Assignee set to Gianluca Petrillo
- Status changed from New to Assigned
The proposal sounds sensible. Please proceed, place any code changes on feature branches, and plan to present at a LArSoft coordination meeting.
#2 Updated by Gianluca Petrillo 10 months ago
- % Done changed from 0 to 100
- transformation into struct
- removal of the empty constructor and explicit initialization of the data members
- data types: two data members changed from
TVector3
togeo::Point_t
- class layout: the
bool
data member moved from the beginning to the end of the class
I have not added the ordering operator (because I have forgotten).
The proposed changes have been implemented in branchesfeature/gp_issue22628
and are now under pull request.The changes happened in:
lardataobj
:sim::OnePhoton
class change [pull request to LArSoft]larsim
: updates for the change (plus an unrelated optimisation) [pull request to LArSoft]ublite
: updates for the change [published as feature branch]
The experiment code for ArgoNeuT (argoneutcode
), MicroBooNE (the rest of uboone_suite
), DUNE (dunetpc
and protoduneana
), SBND (sbndcode
) and ICARUS (icaruscode
) have been verified not to require any change.
Compatibility tests with data files from LArSoft v08_50_02
have been successfully performed with icaruscode
as test bench.
For reference, from a ICARUS cosmic ray sample I have found the improvement in memory (VmHWM):
- from the change to the class (from
TVector3
togeo::Point_t
and change of layout): 8411 MB → 8062 MB - from the side optimisation (writing of
sim::SimEnergyDeposits
data product): 8062 MB → 7790 MB
A serious speed up was also observed, but I honestly can't explain it, did not investigate it nor attempted to reproduce it.
#3 Updated by Gianluca Petrillo 10 months ago
I have fixes a major semantic issue: one of the two points subject of change, FinalLocalPosition
, is not in world coordinates.
I decided to introduce a different type for that point, so that it can't be processed homogeneously with the world coordinate points (geo::Point_t
).
There is some infrastructure for that already, so I have bound the new point type to the one used in geo::OpDetGeo
.
- transformation into struct
- removal of the empty constructor and explicit initialization of the data members
- data types: two data members changed from
TVector3
togeo::Point_t
andgeo::OpticalPoint_t
- class layout: the
bool
data member moved from the beginning to the end of the class - addition of a
sim::OnePhoton
comparison operator geo::OpDetGeo
now also uses typegeo::OpticalPoint_t
as local point type
feature/gp_issue22628
, is:
larcoreobj
: optical vector type definitions [pull request to LArSoft]larcorealg
:geo::OpDetGeo
using optical vector types [pull request to LArSoft]lardataobj
:sim::OnePhoton
class change [pull request to LArSoft]larsim
: updates for the change (plus an unrelated optimisation) [pull request to LArSoft]ublite
: updates for the change [published as feature branch]
The experiment code for ArgoNeuT (argoneutcode
), MicroBooNE (the rest of uboone_suite
), DUNE (dunetpc
and protoduneana
), SBND (sbndcode
) and ICARUS (icaruscode
) have been verified not to require any change.
#4 Updated by Lynn Garren 9 months ago
#5 Updated by Lynn Garren 9 months ago
CI build triggered
trigger --build-delay 0 --cert /tmp/x509up_u1147 --version develop --workflow default_wf --revisions "LArSoft/larcoreobj#7 LArSoft/larcorealg#6 LArSoft/lardataobj#7 LArSoft/larsim#17 ublite@feature/gp_issue22628"
#6 Updated by Lynn Garren 9 months ago
There are two outstanding problems with this set of changes. First there are problems compiling with c7. Second, the uboone CI fails because the ublite feature branch has not been updated to match the head of develop, which means that there is a version mismatch.
#7 Updated by Gianluca Petrillo 9 months ago
Thank you for the feedback.
Clang error¶
I could not test the whole set of experiment packages at once with Clang: I am apparently failing to convince Cmake to use Clang: it seems to pick up GCC 8.2.0.
So I am sticking to testing only LArSoft and MicroBooNE code base.
A single error was reported (inconsistent use of struct
and class
for the same object), which is now fixed.
I have updated the feature branch in lardataobj.
MicroBooNE error¶
I don't fully understand the problem. I have found that my local GIT branch was not tracking a remote one, so I am not positive if the branch for ublite was actually pushed. I have pushed it now, to be safe.
Beside that, I don't know what else is needed: none of the branches is updated with the head of develop
: they are based on LARSOFT_SUITE_v08_51_00
and UBOONE_SUITE_v08_51_00
, respectively.
#8 Updated by Lynn Garren 9 months ago
The branch for ublite needs to be based off the head of develop. Otherwise the CI test simply fails to build due to a mismatch in versions of dependent products.
I used the CI to build everything for c7. A link to the CI output is in the PR comments. Thank you for fixing the problem.
#9 Updated by Lynn Garren 9 months ago
c7 CI tests are now OK. I updated the ublite feature branch myself.
#10 Updated by Lynn Garren 9 months ago
These PR's are now in larsoft v08_53_00.
#11 Updated by Kyle Knoepfel 9 months ago
- Status changed from Assigned to Resolved
#12 Updated by Kyle Knoepfel 9 months ago
- Status changed from Resolved to Closed
Updates for new sim::OnePhoton.
Ripples from issue #22628.