Accommodate breaking changes for geometry refactoring#642
Accommodate breaking changes for geometry refactoring#642leoaliaga merged 6 commits intoSBNSoftware:developfrom
Conversation
knoepfel
force-pushed
the
geom-separate
branch
from
0622542 to
f69332a
Compare
|
trigger build SBNSoftware/icarusalg#75 |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard 🚨 For more details about the failed phase, check the ci_tests ICARUS phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for c14:prof - ignored warnings for build -- details available through the CI dashboard 🚨 For more details about the failed phase, check the ci_tests ICARUS phase logs parent CI build details are available through the CI dashboard |
|
See LArSoft/larpandora#43. It's likely that the CI results will be more successful once that PR is adopted. |
|
trigger build SBNSoftware/icarusalg#75 LArSoft/lar*@LARSOFT_SUITE_v10_01_04 |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard 🚨 For more details about the failed phase, check the ci_tests ICARUS phase logs parent CI build details are available through the CI dashboard |
|
trigger build SBNSoftware/icarusalg#75 LArSoft/lar*@LARSOFT_SUITE_v10_03_01 |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
|
🚨 For more details about the warning phase, check the ci_tests ICARUS phase logs parent CI build details are available through the CI dashboard |
knoepfel
force-pushed
the
geom-separate
branch
from
88238d6 to
626c572
Compare
jzennamo
added
the
breaking
gputnam
left a comment
gputnam
left a comment
There was a problem hiding this comment.
Only caught one possible issue! Although I did not look closely at PMT/CRT code.
There was a problem hiding this comment.
There are a few automatic replacements that should not have happened.
Then there are the more important comments, but GitHub is not friendly in helping me find them, and it must have deleted my draft review already twice, so I'll refer to the comments directly. [after submitting the review I can easily see my own comments all lined up]
I thing there is a bug to be fixed.
One other comment requires the review of @SFBayLaser, whom I am adding to the reviewers and informing of this.
There was a problem hiding this comment.
I think these changes were by an overzealous text replacement. Maybe revert?
There was a problem hiding this comment.
Same overzealous replacement as above.
|
|
||
| //wireID.Wire = int(distanceToWire); | ||
| double distanceToWire = m_wireReadoutAlg->Plane(wireIDIn).WireCoordinate(geo::vect::toPoint(position.data())); | ||
|
|
||
| // Not sure the thinking above but is wrong... switch back to NearestWireID... | ||
| wireID = m_geometry->NearestWireID(geo::vect::toPoint(position.data()),wireIDIn); | ||
| wireID.Wire = int(distanceToWire); |
There was a problem hiding this comment.
My reading of the code is that the former code was using rounding to the closest integer (inside NearestWireID()), while this is explicitly using truncation.
In addition, the original code can throw a (annoying) exception (geo::InvalidWireError, as described in the documentation), while here this will not happen.
That documentation also suggests a possible solution. The author of this code (@SFBayLaser?) should weigh in.
There was a problem hiding this comment.
Yes, the TODO in the documentation would be an ideal solution!
There was a problem hiding this comment.
Ok, I went back to the original file to remind myself... there is a nearly decade long history here (from the early days of cluster3d) where there were issues, obviously, using the WireCoordinate method which were not understood (hence it being commented out). I am pretty sure this last configuration has been in place for around 5-6 years and I'd bet that now it could be done better...
I'd accept the change for now, then we should compare this file against the one in sbncode, reconcile differences and then remove this from icaruscode. (and, really, we should update everything in larreco so it can be used for DUNE too since it seems someone has been picking it up).
SFBayLaser
left a comment
SFBayLaser
left a comment
There was a problem hiding this comment.
I am ok with what I was asked to look at
PetrilloAtWork
left a comment
PetrilloAtWork
left a comment
There was a problem hiding this comment.
I have integrated all the small changes and a bug fix in the branch.
There is still a pending question on a change that looks unnecessary and whose motivation is not clear.
Lacking understanding of the change, I have left the code as proposed by the submitter.
While these aspects are dealt with, I am ok for the PR to proceed.
PetrilloAtWork
left a comment
PetrilloAtWork
left a comment
There was a problem hiding this comment.
Finally rejecting this PR in favour of #808.
github-project-automation
bot
moved this from Partially reviewed
to Done
in SBN software development
8 participants
Accommodate changes from:
N.B.: This PR should not be merged until the above PRs have been merged into a LArSoft release.