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

Changes to enable new DeepTauId trainig #37892

Merged
merged 32 commits into from
May 17, 2022

Conversation

mbluj
Copy link
Contributor

@mbluj mbluj commented May 10, 2022

PR description:

This PR adapts DeepTauId producer to a new v2p5 training. The changes are not significant since the architecture and data preprocessing has not changed much structurally compared to previous version. The changes include:

  • unification of the interface for scaling (aka standardization) of inputs across subversions and reading of scaling constants from a separate namespace,
  • functionality to drop specific global variables from a general list (TauBlockInputs),
  • adaptation of runTauIdMVA.py tool,
  • new deepTauId2018v2p5 producer added to the miniAOD workflow and its products (raw tauIDs) stored in the slimmedTaus collection.

Full list of adaptations needed for the v2p5 can be found here: cms-tau-pog/TauMLTools#99.

In addition this PR consists of changes to DFIsolation producer (1st version of tauID based on DNN) which adapts the producer to changes in base class (previously overlooked).

Note: this PR requires new training files being added to cms-data by cms-data/RecoTauTag-TrainingFiles#8.

PR validation:

Standard tests, i.e. runTheMatrix.py -l limited -i all --ibeos, passed successfully.

Backward-compatibility checks: updated CMSSW code fully reproduces predictions of previous version (v2p1) of deepTauId.

@jpata
Copy link
Contributor

jpata commented May 16, 2022

run3 step4 CPU:  0.93% -> 1.59%
prev: | 0.93 | 0.64 | 0.64 | 1 | 1 | deep_tau::DeepTauBase::produce(edm::Event&, edm::EventSetup const&)
post: | 1.59 | 1.06 | 1.06 | 1 | 1 | deep_tau::DeepTauBase::produce(edm::Event&, edm::EventSetup const&)

run3 step4 eventsize:
   132993 ->      133038         44             0.0     ALL BRANCHES

for future reference. This does not affect the reco signature.

@smuzaffar smuzaffar modified the milestones: CMSSW_12_4_X, CMSSW_12_5_X May 16, 2022
@mbluj
Copy link
Contributor Author

mbluj commented May 16, 2022

This PR is hopefully close to be fully signed, but in meantime its milestone was changed from 12_4_X to 12_5_X. Should we backport this development to have it in 12_4_X? (Technically it will not be a real backport, but a new PR from same dev branch to different target branch.)

@jpata
Copy link
Contributor

jpata commented May 16, 2022

It missed the xpog signature on the weekend from being integrated to 12_4_0_pre4 (a risk one takes with PRs opened close to / on deadlines).
Technically backports of physics-changing quantities are not allowed, as that defeats the purpose of a closed release.

@mbluj
Copy link
Contributor Author

mbluj commented May 16, 2022

It missed the xpog signature on the weekend from being integrated to 12_4_0_pre4 (a risk one takes with PRs opened close to / on deadlines). Technically backports of physics-changing quantities are not allowed, as that defeats the purpose of a closed release.

It is clear, but this development has high importance for all tauID users (different thing than with #37918). In principle we can backport the development to 124X with an appropriate modifier for miniAOD configuration which will fulfill the non-changing policy requirements. @jpata - is there such a modifier already present in 124X? @kandrosov - is this solution acceptable for Tau POG?

@kandrosov
Copy link
Contributor

kandrosov commented May 16, 2022

@perrotta, @dpiparo, @qliphy, @jpata This PR is very important for tau POG and as was discussed it should be part of the default 12_4. Therefore we ask it to be integrated into 12_4 as-it-is, without any miniAOD-modifiers. Taus are at the end of the reconstruction chain, so affected physics-changing quantities are limited.

@jpata
Copy link
Contributor

jpata commented May 16, 2022

@kandrosov not sure what we can do on the reco end at this point (it was signed some days ago, well in time for the release which was built on Sunday).

Have you already checked with XPOG if they had some concerns with this?

@kandrosov
Copy link
Contributor

Have you already checked with XPOG if they had some concerns with this?

we didn't have any feedback from XPOG yet. @mariadalfonso,@gouskos,@fgolf - do you have some concerns with this PR? Are you ok with this PR to be still included in 12_4?

@jpata
Copy link
Contributor

jpata commented May 16, 2022

@mbluj @kandrosov @cms-sw/tau-pog-l2
it might be useful if you connect to the Release Planning meeting tomorrow https://indico.cern.ch/event/1160355/
for a post-mortem, and to understand what the next steps can be.

@mariadalfonso
Copy link
Contributor

+xpog

  1. small changes in the current MVA trainings Changes to enable new DeepTauId trainig #37892 (comment) ascribed to non reproducibility as seen also in previous taus-ID tests Non-reproducibility in DeepTau in 1325.81 #32628
  2. extraID (provisional training in cms-data) added, the v2p1 remains
  3. nano update will follow up in a separate PRs

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@mbluj
Copy link
Contributor Author

mbluj commented May 17, 2022

@perrotta basing on discussion today I understand I "backport" tau PRs to 124X, correct?

@perrotta
Copy link
Contributor

@perrotta basing on discussion today I understand I "backport" tau PRs to 124X, correct?

Yes: you backport those PRs to 124X, and offer a beer next time we meet :-)

@mbluj
Copy link
Contributor Author

mbluj commented May 17, 2022

@perrotta basing on discussion today I understand I "backport" tau PRs to 124X, correct?

Yes: you backport those PRs to 124X, and offer a beer next time we meet :-)

Done.
I will offer you a beer with pleasure, noted :) In fact all tau community owes you - thank you for flexibility!

@perrotta
Copy link
Contributor

+1

@perrotta
Copy link
Contributor

merge

  • Unit test error is also in the IB

@cmsbuild cmsbuild merged commit 0cd6c4f into cms-sw:master May 17, 2022
@cmsbuild cmsbuild mentioned this pull request May 17, 2022
@mbluj mbluj deleted the CMSSW_12_4_X_tau-pog_deepTau-v2p5 branch October 10, 2023 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants