Unexpected and incorrect MCTruthAndBkgCat behaviour
When using MCTruthAndBkgCat in master on a sample produced in Moore there is unexpected and incorrect behaviour.
Running on an Lb2JpsiLambda sample (15144103), the MC truth information (TRUEID etc) for the reconstructed Lb appears identical to the reconstructed Jpsi.
When running over the same inputs and scripts with lb-run DaVinci/latest ... the MC truth information appears as expected.
Example scripts and files can be found at /eos/user/i/isanders/public_lhcb/MCBkgCatBug
Activity
I can confirm this bug. With master, I get:
root [3] Tuple->Scan("Lb_TRUEID:Jpsi_TRUEID:Lambda0_TRUEID:mu_plus_TRUEID:mu_minus_TRUEID:p_plus_TRUEID:pi_minus_TRUEID") ************************************************************************************************ * Row * Lb_TRUEID * Jpsi_TRUE * Lambda0_T * mu_plus_T * mu_minus_ * p_plus_TR * pi_minus_ * ************************************************************************************************ * 0 * 443 * 443 * 3122 * -13 * 13 * 0 * 0 * * 1 * 443 * 443 * 3122 * -13 * 13 * 0 * 0 * * 2 * 443 * 443 * -3122 * 13 * -13 * 0 * 0 * * 3 * 443 * 443 * -3122 * 13 * -13 * 0 * 0 * * 4 * 443 * 443 * 0 * 13 * -13 * 0 * 0 * * 5 * 443 * 443 * 3122 * -13 * 13 * 0 * 0 * * 6 * 443 * 443 * 0 * 13 * -13 * 0 * 0 * * 7 * 443 * 443 * 0 * 13 * -13 * 0 * 0 * ************************************************************************************************Running over the same sample with version 63r4, I get:
root [2] Tuple->Scan("Lb_TRUEID:Jpsi_TRUEID:Lambda0_TRUEID:mu_plus_TRUEID:mu_minus_TRUEID:p_plus_TRUEID:pi_minus_TRUEID") ************************************************************************************************ * Row * Lb_TRUEID * Jpsi_TRUE * Lambda0_T * mu_plus_T * mu_minus_ * p_plus_TR * pi_minus_ * ************************************************************************************************ * 0 * 5122 * 443 * 3122 * -13 * 13 * 2212 * -211 * * 1 * 5122 * 443 * 3122 * -13 * 13 * 2212 * -211 * * 2 * 0 * 443 * -3122 * 13 * -13 * -2212 * 211 * * 3 * -5122 * 443 * -3122 * 13 * -13 * -2212 * 211 * * 4 * 0 * 443 * 0 * 13 * -13 * -2212 * 2212 * * 5 * 5122 * 443 * 3122 * -13 * 13 * 2212 * -211 * * 6 * 0 * 443 * 0 * 13 * -13 * -2212 * 211 * * 7 * 0 * 443 * 0 * 13 * -13 * -211 * 211 * ************************************************************************************************We use the tables at TES location
/Relations/ChargedPP2MCPandDaVinciSmartAssociatortool viaMCTruthAndBkgCatAlg. Not sure what is going on here.Tag relevant people so that they are aware: @pkoppenb , @erodrigu , @nskidmor
I also see some unexpected behaviour of the truth persistence. I run with my DaVinci option (attached) over four trigger lines in a loop, for each saving variable information for two muon daughters and the J/psi mother. However, while all reconstructed information as well as the truth information for one of the muons is always saved, in three of four lines the truth information for the other muon as well as the mother is not stored and the background category is set to 60. Running over each of the lines separately leads to all information being stored sucessfully.I saw this issue first with DaVinci/v63r3 and reproduced it with the nightlies and the current master build since.davinci_nightly.py
Edited by Rowina Caspary
Interesting @isanders . This change of behaviour is something we ought to catch. In fact we've been discussing on checks on the actual ntuple contents in #94 (closed), and a first stab at this work got done in !822 (merged). I will try and move with more checks in the near future ... And we should understand why the behaviour changed, though it is not that surprising given all changes related to persistency that went in recently. This is again a reminder that tests are super important.
@rowina, maybe you should make a separate issue for the problem you found since a bit different. I had no time yet to dig into your script but it does look reasonably standard, hence something we test (modulo content checks, as said above). Now, why are you only reporting now a problem that you observed with a release from a month ago? Never wait to report, that's appreciated ;-).
I only ran into the problem very recently when I started an Analysis Production using that released version of DaVinci. Until then, the older tuples I had produced worked perfectly well, so I also can't say whether older versions of DaVinci releases have the problem or not, unfortunately. I did report the problem in the DPA WP3 mattermost channel as soon as I found it (see https://mattermost.web.cern.ch/lhcb/pl/sf4gg9efgtrkbfik3nrnwopo5r), but got distracted by the issue with the warning that turned out to be completely unrelated. I opened an issue here #100 (closed).
-
I had a look at the related issue #100 (closed) but so far cannot understand. I will try and post some comments soon ... As for the PIDs in the tables you paste above: it is totally weird that on master the protons and pions are no longer matched to MC and then the Lb is given the same TRUEID as the J/psi, as if the relation tables were no longer found/existing and somehow the functor retrieving the TRUEID given a wrong match! Did you look into this at all, @amathad?
I guess we need to start by running the example with the OutputLevel of MCTruthAndBkgCat set to verbose. I set the option
msg_svc_format: str = '% F%60W%S %7W%R%T %0W%M'to be able to see the algo names properly ...BTW, in my investigations I find it difficult to follow nicely the flow of all printouts from the algos and tools instantiated in MCTruthAndBkgCat. At some point we should revisit them, also to modernise the code to make them functional (I know, not a trivial task!).
-
Looking at the diffs between master and released versions, it is reasonably clear that the bug got introduced somewhere upstream, not in Analysis and DaVinci (https://gitlab.cern.ch/lhcb/Analysis/-/compare/v41r4...master?from_project_id=542&straight=false and https://gitlab.cern.ch/lhcb/DaVinci/-/compare/v63r4...master?from_project_id=544&straight=false).
At this point let me cc @graven and @sesen in case they have an idea of where to look.
Can you test with LHCb!3963 (merged)?
to be precise: test with an input file which was produced by a stack which includes LHCb!3963 (merged) (and the two other MR which go with it)
-
The test you suggest might as well wait 1 day for your set of MRs to go in and given that I read earlier today that @nskidmor is producing a new input file. Seems more effective than doing it in parallel the hard way. Now, this cannot explain why the present test jobs we have in DaVinci were giving correct results in the past - files from months back - but no longer now. I could be wrong.
BTW, in the meantime I also had a look at the diffs in Rec and I could not spot anything obvious. Maybe in LHCb indeed ...
@nskidmor, keep us posted for when you have new files. Anyway we said somewhere else - can't remember where - that we need to update several test files. Thanks a lot.
Nicole's new file will only get rid of the warnings. But we don't have any test that tells us if truth matching is done properly in detail. I would suggest @isanders to do the check anyway before we merge LHCb!3963 (merged). The two issues appeared around same time, so likely have the same source.
Thanks for looking into this. I am rebuilding my stack with LHCb!3963 (merged) and will post results here when done
-
That's actually not true anymore given the first batch of work I did with !842 (merged). Believe me, I will be very happy to see some of those fail as clearly for now they contain checks of ntuple contents that we know ae not OK ;-). I will finalise that kind of work so that in the future we have more detailed checks on what exactly the ntuples contain as values of key variables. Happy to receive more ideas there, BTW.
Yes, somehow several things happened at the same time. (I'm trying to help but cannot devote all my time right now, unfortunately.)
Ideally this particular issue gets promoted into an additional integration test -- in general, if there is any new issue discovered that is not covered by the existing tests (and as @sesen writes above, this is the case here) it should be turned into a test so that we increase our coverage. Also, it makes diagnosing/fixing the underlying problem a lot easier (as it becomes even more trivial to reproduce)...
-
You seem to not be reading what I say. I've just said I updated some of the tests to catch these issues, and there is more in the pipeline.
Yes Gerhard I work the way you mention, and for sure the issue reported in the related #100 (closed) needs to be made a test since it runs 4 instances of MC matching where trouble appears, which we did not test at all so far.
If this one here catches something else, we need to add a test as well. In the same vain we also need to duplicate some of the existing tests for data and MC inputs, precisely to increase coverage and be maniac ;-).
-
Still to be more comprehensive - some tests related to packing-unpacking should be done already in Moore, with Sprucing jobs most likely, as that's the natural dataflow. No need to wait for tupling in DaVinci to see some data is missing or not related OR or ... We all agree we're far from such an ideal situation - there's work to be done.
I tested against
LHCb/issue-DaVinci#96,DaVinci/follow-LHCb!3963andAnalysis/follow-LHCb!3963but I still get the same results:************************************************************************************************ * Row * Lb_TRUEID * Jpsi_TRUE * Lambda0_T * mu_plus_T * mu_minus_ * p_plus_TR * pi_minus_ * ************************************************************************************************ * 0 * 443 * 443 * 3122 * -13 * 13 * 0 * 0 * * 1 * 443 * 443 * 3122 * -13 * 13 * 0 * 0 * * 2 * 443 * 443 * -3122 * 13 * -13 * 0 * 0 * * 3 * 443 * 443 * -3122 * 13 * -13 * 0 * 0 * * 4 * 443 * 443 * 0 * 13 * -13 * 0 * 0 * * 5 * 443 * 443 * 3122 * -13 * 13 * 0 * 0 * * 6 * 443 * 443 * 0 * 13 * -13 * 0 * 0 * * 7 * 443 * 443 * 0 * 13 * -13 * 0 * 0 * ************************************************************************************************Edited by Izaac Sanderswoodcan you then also check (after getting rid of the above MR) what happens if you instead use LHCb!3970 (merged)? In that case, there is no need to re-make the input file(s), as the change is to the unpacking only.
-
Hello @isanders, I tried to run your original code posted in the header with the latest DaVinci v63r4, just for a "reference", and get
root [5] Tuple->Scan("Lb_TRUEID:Jpsi_TRUEID:Lambda0_TRUEID:mu_plus_TRUEID:mu_minus_TRUEID:p_plus_TRUEID:pi_minus_TRUEID") ************************************************************************************************ * Row * Lb_TRUEID * Jpsi_TRUE * Lambda0_T * mu_plus_T * mu_minus_ * p_plus_TR * pi_minus_ * ************************************************************************************************ * 0 * 0 * 443 * 0 * -13 * 13 * 211 * -211 * * 1 * 0 * 443 * 310 * -13 * 13 * 211 * -211 * * 2 * 0 * 443 * 0 * -13 * 13 * 211 * -211 * * 3 * 0 * 443 * 310 * 13 * -13 * -211 * 211 * * 4 * 0 * 443 * 0 * -13 * 13 * 2212 * -211 * * 5 * 0 * 443 * 0 * 13 * -13 * -2212 * 211 * * 6 * 5122 * 443 * 3122 * -13 * 13 * 2212 * -211 * ************************************************************************************************which is not at all what you and Abhijit posted above. What's the catch? Did you change input file or ...? Just so that we are on the same page.
Edited by Eduardo Rodrigues -
OK, fine. I went ahead with the latest file. Shame I did not start looking at this MR before looking at #100 (closed) as it seems I got distracted by non-optimal settings that seem not the actual source of troubles.
@graven, the following show some changes and I wonder if your "amalgamation MR in Phys/DaVinciTools got something wrong in there, as it was done post the DaVinci v63r4 release. See the following diff from the first candidate looked at in v63r4 and with the nightlies version:
Version v63r4:
MCTruthAndBkgCatAlg#1 VERBOSE Making relation tables MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Added 2 daughters MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Level: 1 - inserted 2 daughters to get 2 MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Level 1 of 0 : 2 daughters 1 MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Added 2 daughters MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Added 2 daughters MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Level: 2 - inserted 4 daughters to get 6 MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Level 2 of 0 : 4 daughters 1 MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Level: 3 - inserted 0 daughters to get 6 MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Level 3 of 0 : 0 daughters 0 MCTruthAndBkgCatAlg#1.ParticleDescendants DEBUG Reached 3. Returning 6 daughters MCTruthAndBkgCatAlg#1 VERBOSE Looping over the flatted vector of a single input particle and its descendants MCTruthAndBkgCatAlg#1 VERBOSE The particle ID of the input particle (for MC association) is 5122 MCTruthAndBkgCatAlg#1.DaVinciSmartAssociator DEBUG The object of type 'KeyedContainer<LHCb::MCParticle,Containers::KeyedObjectManager<Containers::hashmap> >*' could not be retrieved from TS at address 'MC/Particles' MCTruthAndBkgCatAlg#1.DaVinciSmartAssociator DEBUG The object of type 'KeyedContainer<LHCb::MCParticle,Containers::KeyedObjectManager<Containers::hashmap> >*' has been retrieved from TS at address '/Event/HLT2/MC/Particles' MCTruthAndBkgCatAlg#1.DaVinciSmartAssociator VERBOSE Performing smart association on { particleID : LHCb.ParticleID(5122) measuredMass : 6164.29 measuredMassErr : 0 momentum : (376.8,5758.33,117227,117531) referencePoint : (0.0038,0.1937,-1.1977) momCovMatrix : [ 10689.5 15388.5 520094 521282 15388.5 22521 754712 756435 520094 754712 2.54985e+07 2.55567e+07 521282 756435 2.55567e+07 2.5615e+07 ] posCovMatrix : [ 0.00016641 9.76238e-06 9.71401e-05 9.76238e-06 0.00087616 0.00709164 9.71401e-05 0.00709164 0.070969 ] posMomCovMatrix : [ -0.0527984 -0.00822821 -0.0829877 -0.001739 -0.0853968 -0.33194 0.0109584 -0.0162401 -0.237089 0.011008 -0.016793 -0.198156 ] extraInfo : [ ] TES=/Event/HLT2/Hlt2Lb2JpsiLambda/Particles } MCTruthAndBkgCatAlg#1.DaVinciSmartAssociator VERBOSE Associating a composite particle with pid = 5122Nightlies:
MCTruthAndBkgCatAlg#1 VERBOSE Making relation tables MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Added 2 daughters MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Level: 1 - inserted 2 daughters to get 2 MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Level 1 of 0 : 2 daughters 1 MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Added 2 daughters MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Level: 2 - inserted 2 daughters to get 4 MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Level 2 of 0 : 2 daughters 1 MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Level: 3 - inserted 0 daughters to get 4 MCTruthAndBkgCatAlg#1.ParticleDescendants VERBOSE Level 3 of 0 : 0 daughters 0 MCTruthAndBkgCatAlg#1.ParticleDescendants DEBUG Reached 3. Returning 4 daughters MCTruthAndBkgCatAlg#1 VERBOSE Looping over the flatted vector of a single input particle and its descendants MCTruthAndBkgCatAlg#1 VERBOSE The particle ID of the input particle (for MC association) is 5122 MCTruthAndBkgCatAlg#1.DaVinciSmartAssociator DEBUG The object of type 'KeyedContainer<LHCb::MCParticle,Containers::KeyedObjectManager<Containers::hashmap> >*' could not be retrieved from TS at address 'MC/Particles' MCTruthAndBkgCatAlg#1.DaVinciSmartAssociator DEBUG The object of type 'KeyedContainer<LHCb::MCParticle,Containers::KeyedObjectManager<Containers::hashmap> >*' has been retrieved from TS at address '/Event/HLT2/MC/Particles' MCTruthAndBkgCatAlg#1.DaVinciSmartAssociator VERBOSE Performing smart association on { particleID : LHCb.ParticleID(5122) measuredMass : 6164.29 measuredMassErr : 0 momentum : (376.8,5758.33,117227,117531) referencePoint : (0.0038,0.1937,-1.1977) momCovMatrix : [ 10689.5 15388.5 520094 521282 15388.5 22521 754712 756435 520094 754712 2.54985e+07 2.55567e+07 521282 756435 2.55567e+07 2.5615e+07 ] posCovMatrix : [ 0.00016641 9.76238e-06 9.71401e-05 9.76238e-06 0.00087616 0.00709164 9.71401e-05 0.00709164 0.070969 ] posMomCovMatrix : [ -0.0527984 -0.00822821 -0.0829877 -0.001739 -0.0853968 -0.33194 0.0109584 -0.0162401 -0.237089 0.011008 -0.016793 -0.198156 ] extraInfo : [ ] TES=/Event/HLT2/Hlt2Lb2JpsiLambda/Particles } MCTruthAndBkgCatAlg#1.DaVinciSmartAssociator VERBOSE Associating a composite particle with pid = 5122In short the descendants algo in the tagged release finds the 2 immediate daughters and then their 2+2 daughters but that does not happen with master. I had no time to dive more but any first thoughts?
-
Gerhard, I already replied on that above, twice. Note that the code we're talking about never had tests since years and years! Not my fault. I'm actually working these days on improving the situation, though I have no authorship or responsibility for the code ;-).
I will anyway look into your MR on the amalgamation ... Thanks for your help.
the bug is in this change:
bool ParticleDescendants::addDaughters( const LHCb::Particle::ConstVector& mothers, LHCb::Particle::ConstVector& Parts ) const { - if ( mothers.empty() ) return false; - bool found = false; - for ( LHCb::Particle::ConstVector::const_iterator i = mothers.begin(); i != mothers.end(); ++i ) { - found = ( addDaughters( *i, Parts ) || found ); - } - return found; + return std::any_of( mothers.begin(), mothers.end(), [&]( const auto& i ) { return addDaughters( i, Parts ); } );where the
+short-circuits, but the-does not...
added MC checking label
mentioned in merge request !842 (merged)
mentioned in merge request Rec!3320 (merged)
mentioned in merge request Analysis!959 (merged)
assigned to @erodrigu
-
Hi. I checked with master and things are fixed AFAIC:
In [7]: df.filter(regex=(".*TRUEID")) Out[7]: Jpsi_TRUEID Lambda0_TRUEID Lb_TRUEID mu_minus_TRUEID mu_plus_TRUEID p_plus_TRUEID pi_minus_TRUEID 0 443 0 0 13 -13 211 -211 1 443 310 0 13 -13 211 -211 2 443 0 0 13 -13 211 -211 3 443 310 0 -13 13 -211 211 4 443 0 0 13 -13 2212 -211 5 443 0 0 -13 13 -2212 211 6 443 3122 5122 13 -13 2212 -211Closing as fixed. Feel free to reopen or, probably better, create a new issue if you spot anything fishy in the future.
-
I forgot to day - I will make your reproducer a DaVinciTest unless you do it yourself. Let me know if you @isanders get to it to avoid duplication of work.
In any case could you make available to me the previous test files comment out at
input_files: - 'Lb2JpsiLambda.Moore.dst' # - 'root://xrootd.echo.stfc.ac.uk/lhcb:user/lhcb/user/i/isanders/2022_10/681356/681356841/Lb2JpsiLambda.Moore.dst' annsvc_config: 'Lb2JpsiLambda.Moore.tck.json' # annsvc_config: '/afs/cern.ch/user/i/isanders/gangadir/workspace/isanders/LocalXML/74/0/output/Lb2JpsiLambda.Moore.tck.json'as they seemed nicer - more matches - according to the tables pasted at the beginning of the thread? Thanks.
I got started on it ...
Edited by Eduardo Rodrigues mentioned in issue #100 (closed)
mentioned in merge request !851 (merged)
mentioned in issue #104 (closed)