Hlt2Conf.test_hlt2_check_packed_data_check fails with warnings

Hlt2Conf.test_hlt2_check_packed_data_check fails because of warnings such as

CaloHypoPacker#1                    WARNING Problem with CaloHypo data packing :-
CaloHypoPacker#1                    WARNING   Original CaloHypo key=98 in '/Event/HLT2/LHCb__Converters__Calo__Hypo__v1__fromV2/OutputHypos'
CaloHypoPacker#1                    WARNING {
hypothesis : Photon
lh : -1
position : { 
z :	12752.8
parameters : -352.334, 132.875, 101594
covariance :
[     0.183737   0.0285899     18.3099
     0.0285899    0.185823     215.366
       18.3099     215.366 7.03394e+06 ]
center : -347.98, 138.406
spread :
[      98.7678     34.2858
       34.2858     235.349 ]
 }
 }
CaloHypoPacker#1                    WARNING   Unpacked CaloHypo
CaloHypoPacker#1                    WARNING {
hypothesis : Photon
lh : -1
position : { 
z :	12752.8
parameters : -352.334, 132.875, 101594
covariance :
[     0.183698     0.02859     18.3011
       0.02859    0.185847     215.368
       18.3011     215.368 7.03395e+06 ]
center : -347.98, 138.406
spread :
[      98.7678     34.2838
       34.2838     235.349 ]
 }
 }

They were somehow taking into account in the test validator but in a fragile way that broke after some configuration changes.

#We expect 5 warning messages due to a calo hypo unpacking
#until understood, count them as expected
from Moore.qmtest.exclusions import remove_known_warnings
countErrorLines({"FATAL": 0, "ERROR": 0, "WARNING": 5},
                stdout=remove_known_warnings(stdout))

We need to find the root cause and fix that. In the meantime we could have some more sophisticated way to filter out only these warnings.

/cc @sesen @jonrob

added Persistency failing build labels

Maybe @cmarinbe can comment. One way would be to increase the threshold for the expected differences so the check doesn't complain.

Yes, tweaking the checking seems like the way to go. I'm not sure if just tweaking thresholds would make sense since. For example, off-diagonal covariance elements are discretized relative to the diagonal elements but then the checking compares absolute differences to a threshold... Some thinking is needed there.

I agree we should understand the root cause of the differences, is it related to the precision of the packed data or similar? \cc @graven

perhaps we should discretize/round the elements prior to packing to some a-priori defined reasonable precision, and then compare that on those discretized values packing->unpacking round trips properly...

Where would this be done? Every element seems to require a different precision.

@graven I think this would be hiding / avoiding the issue a bit.

To me there are two things to test:

1. As you note, we should test the packing-unpacking on the discretized values. Maybe it's easier to do something even more constraining, that is, test that the packing is idempotent (pack(X) == pack(unpack(pack(X)))). I think we used to have this kind of test.
2. Check that unpack(pack(X)) is "reasonably close" to X (as is done in the check that fails for CaloHypos). This ensures that, for example, pack(X) does not create only zeros (which would still pass the idempotence test).

1 can be done but for every object type such a check needs to be implemented. At the moment we only have unpacked comparison functions.

mentioned in issue LHCb#206 (closed)

marked this issue as related to LHCb#206 (closed)

removed the relation with LHCb#206 (closed)

marked this issue as related to LHCb#206 (closed)

mentioned in merge request LHCb!3524 (merged)

in LHCb!3524 (merged) I've added some additional diagnostic that prints out which of the comparisons actually goes wrong, and it is not actually the covariance matrix, but the 'energy' part of the 'position', i.e. in the above printout, it is the last element of

parameters : -352.334, 132.875, 101594

which is surprising, as the printout for both packed and unpacked are actually the same.

Printing the difference between them reveals that it is 0.005 which should be compared to the test which requires 5.e-3 < std::abs( oPos->e() - tPos->e() ). So inspired by the difference between 0.005 and 5.e-3 I changed this test to be 5.01e-3 < std::abs( oPos->e() - tPos->e() ) makes the test pass again... and this is consistent with the packing, as it multiplies the value by 1e2, and then stores it as an int, so the rounding to integer could affect things by half of the inverse of the multiplier, i.e 5e-3, hence I think a difference of epsilon more than 0.005 should be allowed. (and note that we're talking about a relative error of a 0.01 MeV on a measurement of 100 GeV, i.e. not only several orders beyond the energy resolution, but more important, I bet it is also way orders of magnitude smaller than any systematic in the energy measurement introduced by eg. the calibration)

assigned to @graven

closed with merge request LHCb!3524 (merged)

mentioned in commit LHCb@51723ba3

mentioned in merge request Rec!2821 (merged)