Accommodate breaking changes for geometry refactoring #75
Conversation
knoepfel
force-pushed
the
geom-separate
branch
from
b8e5788 to
cb596e2
Compare
|
This is related to SBNSoftware/icaruscode#642. |
knoepfel
force-pushed
the
geom-separate
branch
2 times, most recently
from
cafb1e9 to
37691b0
Compare
knoepfel
force-pushed
the
geom-separate
branch
from
37691b0 to
10ae5a6
Compare
Also updating the file format to the refactored one.
PetrilloAtWork
left a comment
PetrilloAtWork
left a comment
There was a problem hiding this comment.
PMT ordering needs to be fixed, and the geometry configuration FHiCL too.
Stand-alone geometry loading needs to be thought through again. I believe there is code here that compiles only because of uninstantiated templates (but I may be wrong).
There are comments spread in the code that are more about upstream design and maybe feature requests than stuff pertaining this specific PR (which is of course affected by that design).
In SBNSoftware there is a fork of this branch with some of the essential fixes already implemented.
|
|
||
| // 1. we initialize it from the configuration in the environment, | ||
| geo::GeometryTestAlg Tester(TestEnvironment.TesterParameters()); | ||
| // 1. we initialize it from the environment, |
There was a problem hiding this comment.
Not really... half the stuff is out of the environment now.
| icarus_geometry_template: { | ||
| SurfaceY: 6.9e2 # in cm, vertical distance to the surface | ||
| Name: "icarus_v2" # see ICARUS wiki for a list | ||
| DisableWiresInG4: true |
There was a problem hiding this comment.
How is the special wireless GDML file served now?
| Name: "icarus_v4" | ||
| GDML: "icarus_refactored_nounderscore_20230918.gdml" |
There was a problem hiding this comment.
As mentioned above, I recommend this to be moved into the template configuration.
| // by plane; the following is still correct, has fewer assumptions and it's | ||
| // easier to expand, but a bit more wordy. | ||
| GeometryCache_t::LimitsCache_t limits = geom.makePlaneData<TimeLimits_t>(); | ||
| GeometryCache_t::LimitsCache_t limits{geom.Ncryostats(), geom.MaxTPCs(), wireReadout.MaxPlanes()}; |
There was a problem hiding this comment.
I suppose makePlaneData() was removed to decouple GeometryCore and WireReadoutGeom from geo::GeoContainerData... I still think being able to create this data structure with a single call is a functionality worth having (there are plenty of places where this would be beneficial... most of them written earlier).
|
@PetrilloAtWork, assuming you are a maintainer, you can commit changes directly to this PR—no need to wait for my approval. This PR was intended to provide a means of starting (if not fully effecting) the migration from LArSoft 9 to 10. It will be much more efficient for ICARUS to directly make the changes to the PR. Otherwise, you will be waiting from me to find spare cycles to implement your suggestions—and those spare cycles are much harder to come by now that almost all of my time is allocated toward framework development for DUNE. |
|
Thank you, @knoepfel. We'll proceed to the changes that we think clear or trivial, and come back to you to discuss the ones that may warrant some thought. |
PetrilloAtWork
left a comment
PetrilloAtWork
left a comment
There was a problem hiding this comment.
I have integrated the simple changes and bug fixes.
There are a couple of points remaining:
- how is the "nowire" geometry served now?
- service provider interface is maximally inconsistent
However, this PR is approved while those points are tackled.
PetrilloAtWork
left a comment
PetrilloAtWork
left a comment
There was a problem hiding this comment.
Finally rejecting this PR in favour of #92.
kjplows
moved this from Open pull requests
to Needs revisiting
in SBN software development
kjplows
moved this from Needs revisiting
to Open pull requests
in SBN software development
4 participants
Accommodate changes from:
N.B.: This PR should not be merged until the above PRs have been merged into a LArSoft release.