Kernel/PartProp: modify LHCb::ParticleID to solve the problems introduced by !1187

@ibelyaev There is something wrong with this MR, it is empty (no changes)

added 1 commit

6dd40646 - fix LHCb::ParticleID

@cattanem sorry.. :-( It should be ok now.

Thanks @ibelyaev. Just a couple of remarks:

We will need also updated Packers/Unpackers (in practice I think we can go back to the original version, but we will now need to support unpacking the version with several data members into this version, as we have been producing data with the new version (in particular Reco18 Brunel output, and soon S34)
Would it be possible to convert the test file you provided into a QMTest, that would immediately spot any regressions in any future development?

As a minor aside, a pet gripe from me and @sponce is that it would be nice to separate formatting changes and actual code changes, it makes review a lot easier...

@cattanem

for the first item, I think, since the visible interface is the same, probably no change in (un)packers is needed at all (for @jonrob to confirm)

for the second item, I can do it, but there is one minor problem - one needs to ignore two lines in reference file (the date and the Git-DB-commit tag). Are there examples to ignore two lines from the output in qmtest?

changed title from Kernel/PartProp: modify LHCb::ParticleID to solve the problmes to Kernel/PartProp: modify LHCb::ParticleID to solve the problems introduced by !1189

changed the description

marked as a Work In Progress

@ibelyaev

For first point, what you say is correct if we had not already merged (and used in some cases) the version with the additional data members. So we have to revert the update to the packers, but taking care to retain support for unpacking the data that contains the additional data members

For the second point, you can explicit exclude some lines from the comparison with the LineSkipper construct, but these CondDB lines are already excluded in the default exclusions file (https://gitlab.cern.ch/lhcb/LHCb/blob/master/GaudiConf/python/GaudiConf/QMTest/LHCbExclusions.py) by defining your validator as follows:

<argument name="validator"><text>
from GaudiConf.QMTest.LHCbExclusions import preprocessor as LHCbPreprocessor
preprocessor = LHCbPreprocessor
validateWithReference(preproc = preprocessor)
</text></argument>

@ibelyaev thanks! Just to make certain from your comment, this passes the scripts/check_ParticleID test? The timing issue was my original concern, but if you think this doesn't really matter, then fine.

I don't think anything needs to be changed w.r.t. the packing. There, only the main PDG code int data member was persisted, and when the packed data is unpacked the constructor from int is called which would in the implementation with the hidden members properly initialise them. So the same code should work in both cases, interchangeably across the two implementations. @cattanem What changes are you referring to in the packing ?

Just to comment on timing, I tend to agree with @ibelyaev that is is not obvious what the impact on timing is with this. Yes, it makes the methods that need to compute things now on the fly a bit slower, but in return it makes the constructor of the class much simpler, just setting an int. As the constructor is generally always used, but the other methods much less so, its quite possible this actually makes things faster for most use cases. Proper profiling would really be the only way to tell how it impacts a particular use case.

@cattaneo And how can I suppreess this difference ?

- [NON-XML-CHAR-0x1B][?1034h# WARNING: Using default tag dddb-20171030-2 for partition DDDB
                Legend:
                -) reference file
                +) standard output of the test

I think the issue is the non-ascii character. If in the reference file you replace it with a # (so that the line begins with # WARNING) it should be ignored by the framework. Or just remove it altogether from the reference file.

added 1 commit

dcdac572 - add qmtest

Compare with previous version

@cattanem Thank you! Removal of this line from reference file does the job! Now the test is ok, added and commit.

@philten Phil I see that PartProp.partprop0 test fails. It is expected? It is a bit strange, since I've generated the reference file using lhcb-head nighlies with your version of the class. If your test is supposed to be valid, I am a bit confused - I am testing all known PIDs + all "exceptional" PIDs. and I see no difference between your code and modified code. It is not clear for me why partprop0 test fails.

added 1 commit

157afc9e - remove commented line

Compare with previous version

How does it fail? I have seen sometimes a failure where just two lines of the output are interchanged.

 output_diff

-  isValid      True
?               ^^^
+  isValid      False
?               ^^^^
-  isNucleus    True
?               ^^^
+  isNucleus    False
?               ^^^^
-  threeCharge  3
?               ^
+  threeCharge  2
?               ^
-  jSpin        1
?               ^
+  jSpin        0
?               ^
-  extra        100
?               ^^^
+  extra        45
?               ^^
-  A            2
?               ^
+  A            0
?               ^
-  Z            1
?               ^
+  Z            0
?               ^
                Legend:
                -) reference file
                +) standard output of the test

 environment

Kernel/PartProp: modify LHCb::ParticleID to solve the problems introduced by !1187

Merge request reports

Activity