RGH Recoil TOF geometry and reconstruction #921
Conversation
niwgit
requested review from
baltzell,
c-dilks,
drewkenjo and
raffaelladevita
as code owners
Co-authored-by: Christopher Dilks <c-dilks@users.noreply.github.com>
|
Great, now it builds and passes most CI tests, except for dependency analysis. We require POMs to specify exactly what dependencies are needed, no more no less. See errors here: https://github.com/JeffersonLab/coatjava/actions/runs/18916433992/job/54001572954?pr=921#step:7:1699 Usually this is straightforward to fix; see other |
|
Thanks. Should I just add "Used undeclared dependencies" to the pom.xml and remove "Unused declared dependencies"? |
Yes, that's usually enough. |
|
Sometimes certain dependencies come through transitively too, so you may need to be a bit more precise with specifying what your code really needs. For example, if A depends on B which depends on C, and your code depends on C, you need to specify C as a dependency, not B (even if both cases lead to successful compilation). |
|
What files should I consider when including dependencies? Is that for import statements in reconstruction/recoiltof/src/main/java/org/jlab/service/recoiltof/RECOILTOFEngine.java? |
The POM file |
|
The java files use "import org.jlab.geom" and "import org.jlab.detector". I used "org.jlab.clas:clas-geometry" and "org.jlab.clas:clas-detector" as dependencies in pom.xml. The compiler says those are unused. What should I change those dependencies to? |
Co-authored-by: Christopher Dilks <c-dilks@users.noreply.github.com>
raffaelladevita
left a comment
raffaelladevita
left a comment
There was a problem hiding this comment.
To avoid conflicts, the suggested changes should be done after #504 is merged and this fork updated
| AHDC (24, "AHDC"), | ||
| ATOF (25, "ATOF"), | ||
| RECOIL (26, "RECOIL"), | ||
| RECOILTOF (27, "RECOILTOF"), |
There was a problem hiding this comment.
it may be better to use an acronim such as RTOF and, similarly, RTRK for the RECOIL
There was a problem hiding this comment.
I suggest reorganizing these classes under the following directory structure:
.../v2/recoil/tof/RecoilTOFConstants.java
.../v2/recoil/trk/...
Also, if constants are the same in the two detectors, a single class could be used
There was a problem hiding this comment.
see the comment above about the directory structure
| }, | ||
| }, | ||
| { | ||
| "name" : "RECOILTOF::tdc", |
There was a problem hiding this comment.
following the suggested change to DetectorType, this should become RTOF
| @@ -0,0 +1,91 @@ | |||
| [ | |||
| { | |||
| "name": "RECOILTOF::hits", | |||
There was a problem hiding this comment.
see the comment above about the name
|
|
||
| DataBank hitbank = this.fillRECOILTOFHitBank(event, barHits); | ||
| if (hitbank != null) { | ||
| event.appendBank(hitbank); |
There was a problem hiding this comment.
it is more efficient to append both banks at once
There was a problem hiding this comment.
I suggest renaming to RTOFCluster
There was a problem hiding this comment.
these constants are already defined in the geometry package and should not be duplicated
units should be cm
There was a problem hiding this comment.
To follow preexisting naming conventions, RECOILTOFHit could become RTOFRawHit and BarHit RTOFHit
| return true; | ||
| } | ||
|
|
||
| DataBank bank = event.getBank("RUN::config"); |
There was a problem hiding this comment.
line 41 to 51 are not necessary
4 participants
This PR includes the geometry and reconstruction for RGH recoil TOF detector.
The reconstruction follows a similar approach as in ATOF.
Running recon-util with engine org.jlab.service.recoiltof.RECOILTOFEngine produces new hipo banks RECOILTOF::hits and RECOILTOF::clusters.