Temporary fix for LArSoft View not correctly set

[marcodeltutto]

In the way the planes are currently implemented in the geometry v02_00, LArSoft cannot interpret the View correctly. Having the View set incorrectly causes problems down the road because many modules (inside and outside of sbndcode) use PlaneID and View interchangeably. This commit swaps plane 0 and plane 1 in TPC 0 only, so that View will be set correctly, but of course ordering of the planes does not match the one in the real detector. Further investigation in LArSoft is needed to fix the View problem, and once fixed, we will revert the planes again to match reality.

Issue #142 has been opened to keep track of this problem.

This "fixes" issue #139.

Here is a comparison of wire angle and View before and after this PR (ThetaZ is angle in radiant w.r.t. Z axis):

Plane, TPC	ThetaZ before	ThetaZ after	View before	View after
0, 0	2.62	0.52	V	U
1, 0	0.52	2.62	U	V
2, 0	1.57	1.57	Y	Y
0, 1	2.62	2.62	U	U
1, 1	0.52	0.52	V	V
2, 1	1.57	1.57	Y	Y

[jzennamo]

The code looks great! It would be nice to see if this fixes the problems that the CI found last time.
@etyley / @henrylay97 would you guys be able to make those plots for this PR?

[pgreen135]

trigger build

[FNALbuild]

CI build for LArSoft on slf7 for e20:prof is in progress -- details available through the CI dashboard

[FNALbuild]

CI build for LArSoft on slf7 for c7:prof is in progress -- details available through the CI dashboard

[FNALbuild]

✔️ CI build for LArSoft Succeeded on slf7 for e20:prof -- details available through the CI dashboard

[FNALbuild]

✔️ CI build for LArSoft Succeeded on slf7 for c7:prof -- details available through the CI dashboard

[FNALbuild]

CI build for SBND on slf7 for c7:prof is in progress -- details available through the CI dashboard

parent CI build details are available through the CI dashboard

[FNALbuild]

CI build for SBND on slf7 for e20:prof is in progress -- details available through the CI dashboard

parent CI build details are available through the CI dashboard

[FNALbuild]

❌ CI build for SBND Failed at phase unit_test SBND on slf7 for e20:prof -- details available through the CI dashboard

🚨 For more details about the failed phase, check the unit_test SBND phase logs

parent CI build details are available through the CI dashboard

[FNALbuild]

❌ CI build for SBND Failed at phase unit_test SBND on slf7 for c7:prof -- details available through the CI dashboard

🚨 For more details about the failed phase, check the unit_test SBND phase logs

parent CI build details are available through the CI dashboard

[pgreen135]

@marcodeltutto this is failing the unit tests for the geometry, I think because its not liking the planes being swapped, logs are linked above

[marcodeltutto]

Thanks @pgreen135, I'll take a look

[marcodeltutto]

The failure is at the unit test stage, when testing geometry_sbnd. I don't think this is a problem with the geometry, but rather with how the test is performed. There is a recent change in larcorealg with PR LArSoft/larcorealg#17 which changes the way this test is done. I tried running a test over the old geometry v01_05 with the latest larcorealg, and this is also failing. More details about the test are given below.

The failure happens when checking that two wires on different TPCs do not intersect. The test uses two wires, the first on TPC 0, plane 0, and the second on TPC 1, plane 1. Then checks the intersection between these two wires. The intersection is checked with the LineClosestPointWithUnitVectors function in larcorealg/Geometry/LineClosestPoint. This algorithm checks that the two wires are not parallel with an assert. In our case, in this PR, these two wires are parallel, which fails the assert. The fact that the wires are parallel though should be ok, because for sure they won't intersect.

https://github.com/LArSoft/larcorealg/blob/a59c4fc47d4c4434eda661435ff8e0aab0b1d23d/larcorealg/Geometry/LineClosestPoint.tcc#L113

@PetrilloAtWork, would you mind commenting on this? Do you think what I say makes sense, or am I off track? Thanks

[PetrilloAtWork]

@marcodeltutto I think you are on the right track. I took a shortcut to find two non-parallel, non-locally-intersecting wires, and, sure enough, five years later this hits me. Or you.
If I remember right, the assert is there because that function assumes the lines do intersect; the test is to see that they do not intersect in the TPC. Anyway, the selection of the wire should be more careful, I'd say.
Care to open a LArSoft issue about that and add me in watch list? Disable the intersection test in the meanwhile (-WireIntersect in the test list, or something like that; if you need help with this, just ask when I have more sleep on my head :-) ).

[henrylay97]

Morning all! I have taken the liberty of pushing a commit to temporarily disable the failing geometry tests as they seem understood from the conversation above. If @pgreen135 can trigger the CI again then assuming all checks pass then I will trigger the validation as well. The validation can't be triggered with any failures in the standard CI.

[pgreen135]

trigger build

[FNALbuild]

CI build for LArSoft on slf7 for e20:prof is in progress -- details available through the CI dashboard

[FNALbuild]

CI build for LArSoft on slf7 for c7:prof is in progress -- details available through the CI dashboard

[FNALbuild]

✔️ CI build for LArSoft Succeeded on slf7 for c7:prof -- details available through the CI dashboard

[FNALbuild]

CI build for SBND on slf7 for c7:prof is in progress -- details available through the CI dashboard

parent CI build details are available through the CI dashboard

[PetrilloAtWork]

@pgreen135 issue 26118 is not issue 26127 is all about this intersection test business though.

[marcodeltutto]

Ah sorry, I linked the wrong LArSoft issue! I meant to link issue 26127.

[PetrilloAtWork]

I still recommend that only the failing test is disabled, or that at least once the geometry test is run to the end to get the understanding of what is broken and what not (hint: nothing should be broken).
If SBND configuration is similar enough to ICARUS, the override should be something like:

#include "test_geometry_sbnd.fcl"

physics.analyzers.geotest.RunTests: [ "@default", "-WireIntersection" ]

[marcodeltutto]

I agree, thanks! Done with the last commit. Opened issue #145 so we don't forget we have disabled this.

[PetrilloAtWork]

A pull request LArSoft/larcorealg#20 is on its way for review.

As I pointed out in that ticket, consider to re-enable the tests that were failing long time ago but are not any more.

[henrylay97]

Thanks both, apologies for my earlier heavy handed removal! I will test reactivating the other tests as you mentioned @PetrilloAtWork and see whether we can have them turned back on permanently.

[jzennamo]

@henrylay97 How is the "full simulation from the group up" going? Running into any issues or anything?

[henrylay97]

@jzennamo it is currently running on the grid. I would tentatively say it should be with you by the end of the working day FNAL time, but its the first time we've run this full workflow so Its quite hard to definitively predict. Its running the sample from gen->analysis hence the slow pace! But no issues yet, running perfectly smoothly!

[jzennamo]

Great to hear! Thank you very much for pushing this through, I wasn't sure if anything caught on fire or not, but it sounds like it is chugging along. Looking forward to the results

[absolution1]

I am about to go on holiday for two weeks so, based on how the changes look, I am happy to slap approval on this now.
Obviously, we should wait for the results of the rerun CI before moving ahead...

[henrylay97]

General update. Results overnight were encouraging but only the nue sample had progressed all the way through to the analysis stage due. The electron dE/dx curve which was the smoking gun of issues before looked much more positive. We are currently rectifying the error with the numu sample and will have a final set of plots within the next few hours. Myself and @etyley will check them and then update you on here. Apologies for the delay.

As I pointed out in that ticket, consider to re-enable the tests that were failing long time ago but are not any more.

On this note, I have tested the rest of the geometry tests that are currently disabled in the fcl file and they all pass using the develop branch so from a CI point of view we're happy to have them brought back in. Thanks @PetrilloAtWork, I will open a separate PR to do so.

[henrylay97]

Hi all! Have the full validation results now. Myself and @etyley are happy with the look of these plots. The original slice purity issue is gone and the dE/dx shape issues we saw as a result of the bug fix have also been removed with this PR.

We're happy for this to now be merged. Thanks for being patient, apologies that it took a little longer than anticipated!

[jzennamo]

Excellent! If github allowed me to post party parrots I would fill the thread with them. Thank you everyone for your hard work on this, I think we can note this as one of the real successes in the system: validation caught a bug, we patched the bug, and we vetted the fix!

[jzennamo]

I think this is now ready to be merged!

[pgreen135]

trigger build

[FNALbuild]

CI build for LArSoft on slf7 for c7:prof is in progress -- details available through the CI dashboard

[FNALbuild]

CI build for LArSoft on slf7 for e20:prof is in progress -- details available through the CI dashboard

[FNALbuild]

✔️ CI build for LArSoft Succeeded on slf7 for c7:prof -- details available through the CI dashboard

[FNALbuild]

✔️ CI build for LArSoft Succeeded on slf7 for e20:prof -- details available through the CI dashboard

[FNALbuild]

CI build for SBND on slf7 for e20:prof is in progress -- details available through the CI dashboard

parent CI build details are available through the CI dashboard

[FNALbuild]

CI build for SBND on slf7 for c7:prof is in progress -- details available through the CI dashboard

parent CI build details are available through the CI dashboard

[FNALbuild]

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for c7:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

[FNALbuild]

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e20:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard