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

Using new Run3ScoutingHitPattern class for hitPattern in Run-3 scouting #35685

Closed
wants to merge 1 commit into from

Conversation

mmasciov
Copy link
Contributor

@mmasciov mmasciov commented Oct 15, 2021

PR description:

This PR is meant to address issue #32219.
It is introducing a new Run3ScoutingHitPattern class in DataFormats/Scouting, to be used for Run-3 scouting (in Run3ScoutingMuon). The class is a "frozen" copy of version 13 (present version) of reco::HitPattern.
A function is defined in order to fill Run3ScoutingHitPattern from reco::HitPattern as well as to define an IO rule for backwards compatibility (with version 3 of Run3ScoutingMuon).

PR validation:

Rerunning the HLT step ("TEST"), and reading hit pattern for "TEST" and for original "HLT" instance, same objects are retrieved:

Events->Scan("Run3ScoutingMuons_hltScoutingMuonPacker__TEST.obj.trk_run3ScoutingHitPattern_.hitCount:Run3ScoutingMuons_hltScoutingMuonPacker__HLT.obj.trk_run3ScoutingHitPattern_.hitCount")
***********************************************
*    Row   * Instance * Run3Scout * Run3Scout *
***********************************************
*        0 *        0 *           *           *
*        1 *        0 *        13 *        13 *
*        2 *        0 *        11 *        11 *
*        2 *        1 *        14 *        14 *
*        3 *        0 *        12 *        12 *
*        4 *        0 *        13 *        13 *
*        5 *        0 *           *           *
*        6 *        0 *        15 *        15 *
*        7 *        0 *           *           *
*        8 *        0 *           *           *
*        9 *        0 *        12 *        12 *
***********************************************

FYI @dsperka @jsalfeld

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35685/25976

  • This PR adds an extra 40KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mmasciov (Mario Masciovecchio) for master.

It involves the following packages:

  • DataFormats/Scouting (core)
  • DataFormats/TrackReco (reconstruction)
  • HLTrigger/Muon (hlt)

@smuzaffar, @Dr15Jones, @makortel, @cmsbuild, @missirol, @Martin-Grunewald, @slava77, @jpata can you please review it and eventually sign? Thanks.
@sscruz, @Fedespring, @missirol, @silviodonato, @trocino, @abbiendi, @JanFSchulte, @jhgoh, @VinInn, @Martin-Grunewald, @calderona, @HuguesBrun, @rovere, @gpetruc, @mmusich, @mtosi, @cericeci this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@makortel
Copy link
Contributor

Adding @davidlange6

nhp.fillRun3ScoutingHitPatternWithHitPattern(onfile.trk_hitPattern_);
trk_run3ScoutingHitPattern_ = nhp;
]]>
</ioread>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would mean making the dependence on DataFormats/TrackReco permanent for DataFormats/Scouting. Given the RAW-like constraints/guarantees for DataFormats/Scouting I'd prefer to avoid that if possible.

(I understood from private discussion with @mmasciov that it is not necessary to be able to read EDM ROOT files with the version 3 of Run3ScoutingMuon; the streamer file format anyway does not support schema evolution)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After checking with @dsperka and the scouting group, it was concluded that indeed it is not necessary to be able to read ROOT files with version 3 of Run3ScoutingMuon.
Will therefore update the PR.

@Martin-Grunewald
Copy link
Contributor

@mmasciov
Are you going to address the comment above?

@mmasciov
Copy link
Contributor Author

@mmasciov Are you going to address the comment above?

@Martin-Grunewald Yes. Please, see also:
#35685 (comment)

@slava77
Copy link
Contributor

slava77 commented Oct 26, 2021

I'm trying to understand where this is going related to DataFormats/Scouting/interface/Run3ScoutingHitPattern.h and Run3ScoutingHitPattern.cc, which are essentially full copies of the DataFormats/TrackReco/interface/HitPattern.h

From the preceding discussion(s) in the scouting meeting I thought that the idea here was to POD-ify the data format of the current version of DataFormats/TrackReco/interface/HitPattern.h for storage. Beyond making a class that would allow to stream this version, I thought that there will just be a conversion module/method that can fill the standard DataFormats/TrackReco/interface/HitPattern.h so that all existing algorithms already using the HitPattern can be reused.

But perhaps I misinterpreted and the goal for the use cases is to have the full copy.

What will happen if we make some updates in DataFormats/TrackReco/interface/HitPattern.h interface (as we did already e.g. #35205 and #35288 ) without changing the persisted data layout? Wouldn't this need to be propagated to Run3ScoutingHitPattern?

BTW, is the current copy in Run3ScoutingHitPattern done after #35288 was merged (Sep 16)?

@mmasciov
Copy link
Contributor Author

I'm trying to understand where this is going related to DataFormats/Scouting/interface/Run3ScoutingHitPattern.h and Run3ScoutingHitPattern.cc, which are essentially full copies of the DataFormats/TrackReco/interface/HitPattern.h

From the preceding discussion(s) in the scouting meeting I thought that the idea here was to POD-ify the data format of the current version of DataFormats/TrackReco/interface/HitPattern.h for storage. Beyond making a class that would allow to stream this version, I thought that there will just be a conversion module/method that can fill the standard DataFormats/TrackReco/interface/HitPattern.h so that all existing algorithms already using the HitPattern can be reused.

But perhaps I misinterpreted and the goal for the use cases is to have the full copy.

What will happen if we make some updates in DataFormats/TrackReco/interface/HitPattern.h interface (as we did already e.g. #35205 and #35288 ) without changing the persisted data layout? Wouldn't this need to be propagated to Run3ScoutingHitPattern?

Yes, this is what I'm currently working on to update this PR: given that we do not need to read the existing scouting samples that made use of HitPattern, I will just do as you describe above.

BTW, is the current copy in Run3ScoutingHitPattern done after #35288 was merged (Sep 16)?

Answer to this is yes, BTW.

@smuzaffar smuzaffar modified the milestones: CMSSW_12_1_X, CMSSW_12_2_X Oct 29, 2021
@slava77
Copy link
Contributor

slava77 commented Nov 1, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 1, 2021

-1

Failed Tests: Build HeaderConsistency ClangBuild
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-713d17/20155/summary.html
COMMIT: 35e09bd
CMSSW: CMSSW_12_2_X_2021-11-01-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35685/20155/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_2_X_2021-11-01-1100/src/DataFormats/PatCandidates/src/Jet.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_2_X_2021-11-01-1100/src/DataFormats/PatCandidates/src/JetCorrFactors.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_2_X_2021-11-01-1100/src/DataFormats/PatCandidates/src/MHT.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_2_X_2021-11-01-1100/src/DataFormats/PatCandidates/src/MET.cc
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_2_X_2021-11-01-1100/src/DataFormats/PatCandidates/src/Muon.cc
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_2_X_2021-11-01-1100/src/DataFormats/PatCandidates/src/Conversion.cc:6:1: error: reference to 'Conversion' is ambiguous
    6 | Conversion::Conversion(int index) : vtxProb_(0.0), lxy_(0.0), nHitsMax_(0) { index_ = index; }
      | ^~~~~~~~~~
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_2_X_2021-11-01-1100/src/DataFormats/PatCandidates/src/Conversion.cc:1:
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_2_X_2021-11-01-1100/src/DataFormats/PatCandidates/interface/Conversion.h:21:9: note: candidates are: 'class pat::Conversion'
   21 |   class Conversion {


Clang Build

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

@makortel
Copy link
Contributor

makortel commented Nov 1, 2021

That is a strange compilation error for this PR.


class TrackerTopology;

using namespace reco;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not allowed in header files global scope

@slava77
Copy link
Contributor

slava77 commented Nov 5, 2021

That is a strange compilation error for this PR.

I think that this is from using namespace reco

@missirol
Copy link
Contributor

Kind ping. I'm wondering if this PR is still targeting 12_2_X (~Nov-30) or not.

@slava77
Copy link
Contributor

slava77 commented Dec 6, 2021

-1

just taking this off the list of currently active PRs for reco
some updates are expected anyways

@makortel
Copy link
Contributor

What is the plan with this PR?

@dsperka
Copy link
Contributor

dsperka commented Jan 31, 2022

@mmasciov should comment. We need to "freeze" the data format for the Run3 MC production soon.

@makortel
Copy link
Contributor

Can this PR be expected to be updated? @mmasciov @dsperka

Or is some other resolution expected for #32219 ?

What is the timescale for freezing the Run 3 scouting data format? (12_3_0? 12_4_0?)

@mmasciov
Copy link
Contributor Author

@dsperka, @makortel, I shall be able to be back on this in the coming days. Will probably close this PR and open a new one at this stage.

About the timescale, I let @dsperka answer as I am not sure.

@dsperka
Copy link
Contributor

dsperka commented Feb 22, 2022

We need to freeze the scouting data format absolute latest by April 15th for the Run 3 MC production.

@makortel
Copy link
Contributor

We need to freeze the scouting data format absolute latest by April 15th for the Run 3 MC production.

Thanks @dsperka. Just curious, could you clarify what is driving the schedule? I'm just wondering because it doesn't seem to correspond to the last open pre-release of either 12_3_X (March 8) or 12_4_X (May 10-15).

@dsperka
Copy link
Contributor

dsperka commented Feb 22, 2022

The TSG set a deadline for the V2 HLT menu 1 month before the 12_4_X deadline

@dsperka
Copy link
Contributor

dsperka commented Mar 2, 2022

@makortel @mmasciov It was mentioned in Today's TSG meeting that data taking will begin already with the V1 menu, and therefore the need to solve this is more urgent (in principle should have been done already). Mario, can you give a precise ETA?

@mmasciov
Copy link
Contributor Author

mmasciov commented Mar 5, 2022

Closing this PR in favor of #37149 (FYI: @dsperka, @makortel).

@mmasciov mmasciov closed this Mar 5, 2022
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.

None yet

8 participants