Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Empty grid for DeepTauId #44501

Open
jfernan2 opened this issue Mar 21, 2024 · 30 comments
Open

Empty grid for DeepTauId #44501

jfernan2 opened this issue Mar 21, 2024 · 30 comments

Comments

@jfernan2
Copy link
Contributor

jfernan2 commented Mar 21, 2024

While implementing basic guards to solve the empty input problem in DeepTauId there were still empty grids which need to be investigated with Tau experts.

#44333 (comment)_

@jfernan2
Copy link
Contributor Author

assign reconstruction

@cmsbuild
Copy link
Contributor

New categories assigned: reconstruction

@jfernan2,@mandrenguyen you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 21, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

A new Issue was created by @jfernan2.

@makortel, @antoniovilela, @rappoccio, @Dr15Jones, @smuzaffar, @sextonkennedy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@jfernan2
Copy link
Contributor Author

type tau

@jfernan2
Copy link
Contributor Author

@cms-sw/tau-pog-l2 FYI

@cmsbuild cmsbuild added the tau label Mar 21, 2024
@jfernan2
Copy link
Contributor Author

@valsdav could you please post here instructions to reproduce the issue in case they had changed from original issue ?
Thank you

@valsdav
Copy link
Contributor

valsdav commented Mar 21, 2024

Hi! Here is the reproducer to get the event with the empty grid.
From #44333 (comment)

  • cmsrel CMSSW_14_0_0; cd CMSSW_14_0_0/src; cmsenv
  • Pick event:
edmCopyPickMerge outputFile=pickevents.root eventsToProcess=367131:196942831 inputFiles=/store/data/Run2023C/DisplacedJet/RAW/v1/000/367/131/00000/9f3f571f-6dc9-4bda-a68b-5d1b9a5fc3ac.root
  • Run:
cmsDriver.py step2 --conditions auto:run3_hlt_relval --data --datatier FEVTDEBUGHLT --era Run3_2023 --eventcontent FEVTDEBUGHLT --filein file:pickevents.root --fileout file:step2.root --nStreams 4 --nThreads 8 --number -1 --process reHLT --python_filename step_2_cfg.py --step L1REPACK:Full,HLT:@relval2024 --accelerators cpu
  • setup deetau log level to 2

You will see empty grids in the input preparation.
If you merge on top my PR #44455 the inference will be skipped on the empty inputs.

@jfernan2
Copy link
Contributor Author

jfernan2 commented May 9, 2024

ping @cms-sw/tau-pog-l2

@jfernan2
Copy link
Contributor Author

ping @cms-sw/tau-pog-l2 @mbluj @Ksavva1021

@mbluj
Copy link
Contributor

mbluj commented May 20, 2024

@kandrosov @brallmond could you also take a look, please?

@kandrosov
Copy link
Contributor

kandrosov commented May 31, 2024

@mbluj I have identified the tau that has the empty signal grid in the example event pointed out above:

tau pt=52.9444 eta=-0.0898154 phi=-0.68437
innerSignalConeRadius = 0.0566632
signalTauChargedHadronCandidates
  pt = 32.8809, eta = -0.177717, phi = -0.669344, mass = 0.13957, deltaR = 0.0892
    charge = 0 (pdgId = 130)
    charged PFCandidate (3:2826:1039): pt = 32.8809, eta = -0.177717, phi = -0.669344 (pdgId = 130)
    reco::Track: N/A
    neutral PFCandidates: N/A
    position@ECAL entrance: x = 163.066, y = -129.112, z = -44.1041 (eta = -0.21049, phi = -0.669706)
    algo = PFNeutralHadron
signalPiZeroCandidates
  pt = 11.7301, eta = 0.0901726, phi = -0.745689
    daughter #0 (PFGamma): pt = 3.92827, eta = 0.0625999, phi = -0.757536, deltaR = 0.169
    daughter #1 (PFGamma): pt = 2.98864, eta = 0.0810623, phi = -0.714655, deltaR = 0.174
    daughter #2 (PFGamma): pt = 3.44201, eta = 0.116514, phi = -0.774948, deltaR = 0.225
    daughter #3 (PFGamma): pt = 1.37547, eta = 0.122283, phi = -0.70606, deltaR = 0.213
  pt = 7.9762, eta = 0.0110623, phi = -0.656186
    daughter #0 (PFGamma): pt = 7.9762, eta = 0.0110623, phi = -0.656186, deltaR = 0.105

Indeed all signal PF candidates have deltaR > innerSignalConeRadius, where innerSignalConeRadius is computed with getInnerSignalConeRadius.
This happens for HLT path HLT_VBF_DiPFJet45_Mjj550_MediumDeepTauPFTauHPS45_L2NN_eta2p1_v2, which uses as an input hltHpsPFTauProducer -> hltHpsPFTauProducerSansRefs -> hltHpsCombinatoricRecoTaus.
In hltHpsCombinatoricRecoTaus signalConeSize = cms.string('max(min(0.1, 3.0/pt()), 0.05)'). So I don't understand how it is possible.

@mbluj
Copy link
Contributor

mbluj commented Jun 3, 2024

@kandrosov thank you for checking. It is indeed very weird tau candidate with one charged hadron on top of neutral one (charged w/o track recovery) and two pi0 candidates (strips) - such candidates are not allowed offline. To be checked if it is allowed online. It is in addition to the fact that tau signal candidates are not in the signal cone of dR~0.0566 as you found.

@kandrosov
Copy link
Contributor

@mbluj, thank you for the confirmation. Tau HPS DM finding selection is not applied online because it would require dedicated tuning. So tau with the worst quality is expected to arrive at the DeepTau step. I was just not expecting that it could be that bad. Given that we are moving to PNet-based tau tagging for online, I think it is not necessary to invest time in re-optimizing HPS for online.
On the other hand, the fix on the code side was implemented some time ago, so DeepTau doesn't crash when processing taus with an empty signal cone. We also understood that the origin of the behavior is not related to any bugs but rather to a very loose HPS selection criterion at the tau building step. I think the GitHub issue can be closed.

@mbluj
Copy link
Contributor

mbluj commented Jun 4, 2024

@mbluj, thank you for the confirmation. Tau HPS DM finding selection is not applied online because it would require dedicated tuning. So tau with the worst quality is expected to arrive at the DeepTau step. I was just not expecting that it could be that bad. Given that we are moving to PNet-based tau tagging for online, I think it is not necessary to invest time in re-optimizing HPS for online. On the other hand, the fix on the code side was implemented some time ago, so DeepTau doesn't crash when processing taus with an empty signal cone. We also understood that the origin of the behavior is not related to any bugs but rather to a very loose HPS selection criterion at the tau building step. I think the GitHub issue can be closed.

I agree that the issue can be closed.
For records: at building step all possible decay modes are built and then the "best one" is selected for further processing. It could be that the "best one" is not passing HPS requirements as in this particular case, but it is strange that all tau products (signal candidates) are outside signal cone - it could be a "feature" built in the algorithm which was not expected to be used without decay mode discriminator, but it should be understood.

@jfernan2
Copy link
Contributor Author

jfernan2 commented Jun 4, 2024

+1
Closing since online is going to use PNet/based tau tagging

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 4, 2024

This issue is fully signed and ready to be closed.

@mmusich
Copy link
Contributor

mmusich commented Jun 4, 2024

Given that we are moving to PNet-based tau tagging for online, I think it is not necessary to invest time in re-optimizing HPS for online.

checking in the very latest development HLT table (/dev/CMSSW_14_0_0/HLT/V151) I see:

$ ./getListOfPathsContaining.py hltHpsPFTauDeepTauProducerForVBFIsoTau /dev/CMSSW_14_0_0/HLT/V151 
--------------------
Paths
--------------------
HLT_IsoMu24_eta2p1_MediumDeepTauPFTauHPS20_eta2p1_SingleL1_v9
HLT_IsoMu24_eta2p1_MediumDeepTauPFTauHPS45_L2NN_eta2p1_CrossL1_v9
HLT_VBF_DiPFJet45_Mjj650_MediumDeepTauPFTauHPS45_L2NN_eta2p1_v3
HLT_VBF_DiPFJet45_Mjj750_MediumDeepTauPFTauHPS45_L2NN_eta2p1_v3
HLT_VBF_DoubleMediumDeepTauPFTauHPS20_eta2p1_v10
$ ./getListOfPathsContaining.py hltHpsPFTauDeepTauProducer /dev/CMSSW_14_0_0/HLT/V151
--------------------
Paths
--------------------
HLT_DoubleMediumDeepTauPFTauHPS30_L2NN_eta2p1_OneProng_v5
HLT_DoubleMediumDeepTauPFTauHPS30_L2NN_eta2p1_PFJet60_v9
HLT_DoubleMediumDeepTauPFTauHPS30_L2NN_eta2p1_PFJet75_v9
HLT_DoubleMediumDeepTauPFTauHPS35_L2NN_eta2p1_v9
HLT_Ele24_eta2p1_WPTight_Gsf_LooseDeepTauPFTauHPS30_eta2p1_CrossL1_v10
HLT_IsoMu20_eta2p1_LooseDeepTauPFTauHPS27_eta2p1_CrossL1_v10
HLT_IsoMu24_eta2p1_LooseDeepTauPFTauHPS180_eta2p1_v10
HLT_IsoMu24_eta2p1_LooseDeepTauPFTauHPS30_eta2p1_CrossL1_v10
HLT_IsoMu24_eta2p1_MediumDeepTauPFTauHPS30_L2NN_eta2p1_CrossL1_v9
HLT_IsoMu24_eta2p1_MediumDeepTauPFTauHPS30_L2NN_eta2p1_OneProng_CrossL1_v5
HLT_IsoMu24_eta2p1_MediumDeepTauPFTauHPS30_L2NN_eta2p1_PFJet60_CrossL1_v9
HLT_IsoMu24_eta2p1_MediumDeepTauPFTauHPS30_L2NN_eta2p1_PFJet75_CrossL1_v9
HLT_IsoMu24_eta2p1_MediumDeepTauPFTauHPS35_L2NN_eta2p1_CrossL1_v10
HLT_LooseDeepTauPFTauHPS180_L2NN_eta2p1_v10

what I am missing?

@brallmond
Copy link
Contributor

Hi Marco,
Originally we expected the DeepTau paths to be prescaled to zero shortly after the first 2024 data. Following the first STEAM meeting using 2024C data, the decision was to collect more data and continue studies. These will be presented at the STEAM meeting on Friday this week.
From the Tau Trigger side, we have asked for feedback regarding DeepTau and PNet comparisons and no one has raised any issues. What we see in our monitoring paths are the same or better performance from the PNet paths. So, my understanding is that TSG/STEAM will make a decision this Friday regarding the prescaling of the DeepTau paths.

@mmusich
Copy link
Contributor

mmusich commented Jun 4, 2024

So, my understanding is that TSG/STEAM will make a decision this Friday regarding the prescaling of the DeepTau paths.

what about MC? Some of these triggers took data (and we normally include those in the frozen table for MC).

@mmusich
Copy link
Contributor

mmusich commented Jun 4, 2024

@jfernan2 please re-open this issue that was closed too hastily. I think something still needs to be done for the HLT step of 2024 MC (or it is clarified it's not needed)

@jfernan2
Copy link
Contributor Author

jfernan2 commented Jun 4, 2024

assign hlt

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 4, 2024

New categories assigned: hlt

@Martin-Grunewald,@mmusich you have been requested to review this Pull request/Issue and eventually sign? Thanks

@jfernan2
Copy link
Contributor Author

jfernan2 commented Jun 4, 2024

Done

@jfernan2
Copy link
Contributor Author

jfernan2 commented Jun 4, 2024

It seems reopening command is not working, let's iterate here anyways

@brallmond
Copy link
Contributor

@jfernan2 please re-open this issue that was closed too hastily. I think something still needs to be done for the HLT step of 2024 MC (or it is clarified it's not needed)

Sorry, but what needs to be done still?
This issue was opened to understand why crashes were occurring online related to this module, and now we have an explanation thanks to Konstantin and Michal. The handling of the corner cases which caused crashes is already taken care of by guards in the parent issue. As far as I understand, it should be no issue for the DeepTau paths to be prescaled in the online menu and remain in the frozen HLT table for 2024 MC since the original issue is handled and understood, and because the DeepTau paths have taken some data now in 2024.

@makortel
Copy link
Contributor

makortel commented Jun 4, 2024

@cmsbuild, please reopen

@cmsbuild cmsbuild reopened this Jun 4, 2024
@mmusich
Copy link
Contributor

mmusich commented Jun 4, 2024

Sorry, but what needs to be done still?

From the mother issue:

Basic guards to solve the empty input problem in DeepTauId are in place, but the reason of the empty grid needs to be investigated with Tau experts.

and from the protection PR:

This PR avoids calling the TF inference if empty inputs are detected.
The grid should never be empty. The issue will be followed up with Tau experts (enter here issue number..)

from the discussion above it's not clear to me (sorry I am not a tau expert) if the workaround in place is discarding events that are potentially useful or not.

@mbluj
Copy link
Contributor

mbluj commented Jun 4, 2024

The "workaround" discards weird, ill-defined, taus which anyway should be filtered out. At offline such taus are killed by basic filter, so-called new decay mode finder, which was omitted at HLT to boost efficiency (due to difference in online reco it would require special tuning) . What still needs be understood is why such ill-defined taus are produced (if it is expected or non expected feature of HPS reconstruction), but I think it is beyond scope of this issue.

@mmusich
Copy link
Contributor

mmusich commented Jun 4, 2024

New episode of the saga at #45136

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants