Iss742: Add condition for SVT sync status #750

JeremyMcCormick · 2020-11-17T05:51:42Z

The SVT sync status condition is represented by the class SvtSyncStatus with the method isGood() returning true if the status is good and false if not.

Data is contained in the table svt_sync_statuses in the primary JLAB conditions database and was loaded from the information provided by @normangraf (see comments on the github issue).

There is a test that performs a spot check on a few of the records, which can be run as follows:

mvn test -Dtest=SvtSyncStatusesTest

This successfully checks that the first and last conditions records are correct (good and bad, respectively).

normangraf · 2021-01-08T20:52:18Z

RawTrackerHitFitterDriver.java fails the style checker and does not compile. Please fix.

mholtrop · 2021-01-12T17:57:37Z

tracking/src/main/java/org/hps/recon/tracking/RawTrackerHitFitterDriver.java

@@ -183,9 +194,17 @@ public void process(EventHeader event) {
                    fit.setT0(fit.getT0() - timingConstants.getOffsetTime());
                }
                if (subtractTriggerTime) {
-                    double tt = (((event.getTimeStamp() - 4 * timingConstants.getOffsetPhase()) % 24) - 12);
-                    if (debug)
+                    double tt = (((event.getTimeStamp() - 4 * timingConstants.getOffsetPhase()) % 24) - 14);


This looks like it changes the tt offset by 2 from what was done previously. I would expect at least some comment why this new behavior is superior than what was done before.

This was discussed in multiple meetings. This shifts the average offset to be roughly zero, rather than the -2 it used to be. The intention was always to have a average time of roughly zero, and not -2.

I would like to see a comment as to why 12 was chosen in the first place, and why it wasn't immediately fixed when it was seen that the behavior of the code was not what was actually intended.

When I brought up it being at -2, it was argued it didn't matter because it changes the times the same amount and we only use relative hit times, not the absolute times, in our selection. I would say the same argument applies to this scenario, and zero is better because then defining a cut on the absolute time becomes a more simple cut.

normangraf · 2021-01-12T19:48:30Z

All of the 2015 & 2016 integration tests fail. This should not happen, as this issue was intended to resolve problems specific to the 2019 data.

cbravo135 · 2021-01-12T19:56:04Z

@normangraf this is an inaccurate representation of the intention of this issue.

cbravo135 · 2021-01-12T19:57:17Z

The intention was to improve the calibration of the hit time corrections.

JeremyMcCormick · 2021-01-12T20:01:36Z

Can you paste the log from the test failures here?

…

On Tue, Jan 12, 2021 at 1:48 PM normangraf ***@***.***> wrote: All of the 2015 & 2016 integration tests fail. This should not happen, as this issue was intended to resolve problems specific to the 2019 data. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#750 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQXV46XK3D3ZDKMRGOKQTDSZSRR3ANCNFSM4TYDNZYA> .

cbravo135 · 2021-01-12T20:02:14Z

https://srs.slac.stanford.edu/hudson/view/HPS/job/hps-java-dev/62/testReport/

normangraf · 2021-01-12T20:03:28Z

Failed tests:
EngRun2015FeeReconTest.testIt:67->comparePlots:106 expected:<1> but was:<103>
EngRun2015MollerReconTest.testIt:62->comparePlots:101 expected:<417> but was:<389>
EngRun2015V0ReconTest.testIt:66->comparePlots:105 expected:<1949> but was:<1957>
PhysRun2016FeeReconTest.testIt:62->comparePlots:89 expected:<125> but was:<128>
PhysRun2016MollerReconTest.testIt:66->comparePlots:93 expected:<2221> but was:<2182>
PhysRun2016V0ReconTest.testIt:66->comparePlots:93 expected:<4890> but was:<4743>

JeremyMcCormick · 2021-01-12T20:41:25Z

This must be a consequence of changing the raw tracker hit fitting. Just adding the sync condition would not do this.

…

On Tue, Jan 12, 2021 at 2:03 PM normangraf ***@***.***> wrote: Failed tests: EngRun2015FeeReconTest.testIt:67->comparePlots:106 expected:<1> but was:<103> EngRun2015MollerReconTest.testIt:62->comparePlots:101 expected:<417> but was:<389> EngRun2015V0ReconTest.testIt:66->comparePlots:105 expected:<1949> but was:<1957> PhysRun2016FeeReconTest.testIt:62->comparePlots:89 expected:<125> but was:<128> PhysRun2016MollerReconTest.testIt:66->comparePlots:93 expected:<2221> but was:<2182> PhysRun2016V0ReconTest.testIt:66->comparePlots:93 expected:<4890> but was:<4743> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#750 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQXV45AR2ITW6RDJVDUWADSZSTJ7ANCNFSM4TYDNZYA> .

normangraf · 2021-01-12T21:48:46Z

That's correct. Just adding the SVT conditions should have no effect.

cbravo135 · 2021-01-12T21:50:27Z

If you look in jenkins, you can see that if I change the 14 back to 12, it passes everything.

cbravo135 · 2021-01-12T21:53:18Z

This is the same code, just with the 14 changed to a 12: https://srs.slac.stanford.edu/hudson/job/hps-java-dev/61/

normangraf

Please revert the offset to 12, after which we can approve this pull request and merge with master. If needed, we can open a new issue to deal with the question of whether the offset should be 12 or 14.

cbravo135 · 2021-01-13T01:36:06Z

We already did discuss changing this to 14 and the analysis group agreed the intention of this number is to put the average hit time at zero. The offset should be 14, and always should have been 14.

cbravo135 · 2021-01-13T01:43:47Z

Mathematically, it makes sense for it to be 14 since the trigger fw and code is in essence "rounding up" the time to the next trigger clock cycle, so it will be off by half of a 4 ns cycle (2 ns) plus the time to the center (from the later edge) of the svt trigger window which is 24 ns wide, so 2 ns plus 12 ns is 14 ns.

cbravo135 · 2021-01-13T01:47:53Z

12 is a bug, and I don't want to change my code to have a bug over not having one.

cbravo135 · 2021-01-13T01:53:18Z

Seems like it is the tests that should change, not the 14.

mholtrop · 2021-01-13T02:12:21Z

I agree, the test should change. However, we cannot merge this until that is done. Can the test change be part of the pull request, or is that separate?

cbravo135 · 2021-01-13T02:53:33Z

The tests can change as part of this PR. I just don't know how to do it myself, so someone that understands how the test framework works would need to do it.

conditions/src/main/java/org/hps/conditions/svt/SvtSyncStatus.java

cbravo135 · 2021-01-25T18:23:30Z

So, after reading the code for the Recon tests that are failing, we just need to update some of the reference files in: http://www.lcsim.org/test/hps-java/referencePlots/
Not sure who has perms to do this, but I have no idea how to upload anything there.
It does seem that there is no need to change any code to get the tests to pass, so we can safely merge this PR.

cbravo135 · 2021-01-26T23:53:12Z

The tests are now changed and legacy drivers were added for 2015/2016 recon.

Tests were modified.

JeremyMcCormick added 2 commits November 16, 2020 18:43

Add class for SVT sync status.

05ce83f

Add test for SVT sync status condition.

1e1acde

JeremyMcCormick added this to the V4.5 milestone Nov 17, 2020

JeremyMcCormick requested review from cbravo135 and normangraf November 17, 2020 05:51

Remove unused import.

6ca9801

JeremyMcCormick linked an issue Nov 25, 2020 that may be closed by this pull request

Add condition for SVT sync status #742

Closed

Adjust time correction for good daq sync vs bad

5a0ab97

Fix indentation style

1cd12b8

mholtrop reviewed Jan 12, 2021

View reviewed changes

normangraf previously requested changes Jan 13, 2021

View reviewed changes

cbravo135 reviewed Jan 18, 2021

View reviewed changes

conditions/src/main/java/org/hps/conditions/svt/SvtSyncStatus.java Outdated Show resolved Hide resolved

Java type casting exception fix

d20fefc

cbravo135 requested review from normangraf, mholtrop, bloodyyugo, cbravo135 and pbutti January 25, 2021 21:19

cbravo135 added 2 commits January 26, 2021 11:18

Make the trigger time offset configurable in the steering file

45412bc

Update to legacy integration tests

2465f48

Merge branch 'master' of github.com:JeffersonLab/hps-java into iss742

0de3396

cbravo135 approved these changes Feb 3, 2021

View reviewed changes

JeremyMcCormick closed this Feb 4, 2021

cbravo135 reopened this Feb 4, 2021

JeremyMcCormick closed this Feb 4, 2021

cbravo135 reopened this Feb 4, 2021

cbravo135 closed this Feb 4, 2021

cbravo135 reopened this Feb 4, 2021

JeffersonLab deleted a comment from cbravo135 Feb 4, 2021

mholtrop approved these changes Feb 5, 2021

View reviewed changes

cbravo135 merged commit bfb2496 into master Feb 5, 2021

JeremyMcCormick modified the milestones: V4.5, v5.0 Feb 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iss742: Add condition for SVT sync status #750

Iss742: Add condition for SVT sync status #750

JeremyMcCormick commented Nov 17, 2020

normangraf commented Jan 8, 2021

mholtrop Jan 12, 2021

cbravo135 Jan 12, 2021

cbravo135 Jan 12, 2021

cbravo135 Jan 12, 2021

normangraf commented Jan 12, 2021

cbravo135 commented Jan 12, 2021

cbravo135 commented Jan 12, 2021

JeremyMcCormick commented Jan 12, 2021 via email

cbravo135 commented Jan 12, 2021

normangraf commented Jan 12, 2021

JeremyMcCormick commented Jan 12, 2021 via email

normangraf commented Jan 12, 2021

cbravo135 commented Jan 12, 2021

cbravo135 commented Jan 12, 2021 •

edited

Loading

normangraf left a comment

cbravo135 commented Jan 13, 2021

cbravo135 commented Jan 13, 2021 •

edited

Loading

cbravo135 commented Jan 13, 2021

cbravo135 commented Jan 13, 2021

mholtrop commented Jan 13, 2021

cbravo135 commented Jan 13, 2021

cbravo135 commented Jan 25, 2021 •

edited

Loading

cbravo135 commented Jan 26, 2021

Iss742: Add condition for SVT sync status #750

Iss742: Add condition for SVT sync status #750

Conversation

JeremyMcCormick commented Nov 17, 2020

normangraf commented Jan 8, 2021

mholtrop Jan 12, 2021

Choose a reason for hiding this comment

cbravo135 Jan 12, 2021

Choose a reason for hiding this comment

cbravo135 Jan 12, 2021

Choose a reason for hiding this comment

cbravo135 Jan 12, 2021

Choose a reason for hiding this comment

normangraf commented Jan 12, 2021

cbravo135 commented Jan 12, 2021

cbravo135 commented Jan 12, 2021

JeremyMcCormick commented Jan 12, 2021 via email

cbravo135 commented Jan 12, 2021

normangraf commented Jan 12, 2021

JeremyMcCormick commented Jan 12, 2021 via email

normangraf commented Jan 12, 2021

cbravo135 commented Jan 12, 2021

cbravo135 commented Jan 12, 2021 • edited Loading

normangraf left a comment

Choose a reason for hiding this comment

cbravo135 commented Jan 13, 2021

cbravo135 commented Jan 13, 2021 • edited Loading

cbravo135 commented Jan 13, 2021

cbravo135 commented Jan 13, 2021

mholtrop commented Jan 13, 2021

cbravo135 commented Jan 13, 2021

cbravo135 commented Jan 25, 2021 • edited Loading

cbravo135 commented Jan 26, 2021

cbravo135 commented Jan 12, 2021 •

edited

Loading

cbravo135 commented Jan 13, 2021 •

edited

Loading

cbravo135 commented Jan 25, 2021 •

edited

Loading