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

Support for Phase-2 mkFit initialStep track building #44708

Merged
merged 4 commits into from
Apr 24, 2024

Conversation

mmasciov
Copy link
Contributor

@mmasciov mmasciov commented Apr 11, 2024

PR description:

This PR adds the ability to run mkFit on Phase-2 data / geometry. Only initialStep iteration is supported so far, with a dedicated workflow (24834.702).

  • Add CMSSW Phase-2 hit and seed converters.

  • Properly support binary readout for Phase-2 strips where cluster-charge cut can
    not be applied.

  • Improve standalone performance monitoring & debugging tools.

It requires also a new JSON file in cmssw/data repository:
cms-data/RecoTracker-MkFit#14

PR validation:

In TTbar events, with PU:

FYI: @osschar, @slava77, @kskovpen

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 11, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44708/39914

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mmasciov for master.

It involves the following packages:

  • Configuration/PyReleaseValidation (upgrade, pdmv)
  • RecoTracker/IterativeTracking (reconstruction)
  • RecoTracker/MkFit (reconstruction)
  • RecoTracker/MkFitCMS (reconstruction)
  • RecoTracker/MkFitCore (reconstruction)

@srimanob, @miquork, @AdrianoDee, @subirsarkar, @cmsbuild, @sunilUIET, @jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks.
@missirol, @VinInn, @JanFSchulte, @gpetruc, @VourMa, @dgulhan, @mmusich, @rovere, @makortel, @Martin-Grunewald, @GiacomoSguazzoni, @mtosi, @fabiocos, @felicepantaleo, @slomeo this is something you requested to watch as well.
@rappoccio, @sextonkennedy, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

@mandrenguyen
Copy link
Contributor

test parameters:

@mandrenguyen
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: ClangBuild
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fb6282/38802/summary.html
COMMIT: 8155977
CMSSW: CMSSW_14_1_X_2024-04-11-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/44708/38802/install.sh to create a dev area with all the needed externals and cmssw changes.

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.

@slava77
Copy link
Contributor

slava77 commented Apr 12, 2024

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

RecoTracker/MkFitCore/interface/IdxChi2List.h:21:18: 
warning: definition of implicit copy constructor for 'IdxChi2List' is deprecated 
because it has a user-declared copy assignment operator [-Wdeprecated-copy]

@osschar this is probably for you

@slava77
Copy link
Contributor

slava77 commented Apr 12, 2024

RecoTracker/MkFitCore/interface/IdxChi2List.h:21:18: 
warning: definition of implicit copy constructor for 'IdxChi2List' is deprecated 
because it has a user-declared copy assignment operator [-Wdeprecated-copy]

uhm, or does this message just mean we need also IdxChi2List(IdxChi2List const& ) = default;

if everything is default, is there even a point to list anything?

@makortel
Copy link
Contributor

uhm, or does this message just mean we need also IdxChi2List(IdxChi2List const& ) = default;

Correct. Or remove the definition copy assignment operator.

if everything is default, is there even a point to list anything?

Generally speaking no, there is no point in default-defining member functions where it is clear the compiler generates the expected definitions implicitly (i.e. the "rule of 5" still holds). Elsewhere we have generally preferred to remove the default-defined copy constructor or assignment operator when only one of those was defined.

@slava77
Copy link
Contributor

slava77 commented Apr 19, 2024

@cms-sw/pdmv-l2 @cms-sw/upgrade-l2 your signature is needed for this PR.
Please check.
It would be nice to get this to 14_1_0_pre3 (planned next week, if I understand correctly)

@@ -437,6 +437,28 @@ def condition_(self, fragment, stepList, key, hasHarvest):
'--procModifiers': 'trackingMkFitDevel'
}

# mkFit for phase-2 initialStep tracking
class UpgradeWorkflow_trackingMkFitPhase2(UpgradeWorkflowTracking):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a different class (and then a different offset)? I don't see anything that could not have been done with UpgradeWorkflow_trackingMkFit as is using the same *.7 offset. Or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Ok, not with the same suffix but within the same class yes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Phase-2 would need a different process modifier, and should not modify step2 on the other hand.
That's why we went with a different class, because it was the simplest solution. We may review this (and remove the class) when we'l have more than one iteration working in phase-2, so that we can reduce otherwise-needed modifications to the mkFit procModifiers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, this was clear. And yes, most probably it's easier to have this rather than "disabling" the other iterations with something like ~phase2_tracker. Maybe I would have inverted the logic here (so a UpgradeWorkflow_trackingMkFitInitialStep) just to have it clear what's happening in the wf from the name. Anyway as you say, this will need to be reviewed anyway later.

@AdrianoDee
Copy link
Contributor

+pdmv
(discussion in https://github.com/cms-sw/cmssw/pull/44708/files#r1572627562 for a future setup)

@slava77
Copy link
Contributor

slava77 commented Apr 22, 2024

@cms-sw/upgrade-l2 your signature is needed for this PR.
Please check.

I'm not sure @AdrianoDee still has the upgrade signature rights ;)

@slava77
Copy link
Contributor

slava77 commented Apr 23, 2024

@cms-sw/upgrade-l2 your signature is needed for this PR. Please check.

@cms-sw/orp-l2 @rappoccio
please comment if this can make it to pre3.
Thank you.

@srimanob
Copy link
Contributor

Hi @mmasciov @slava77 @mmusich
The readme should also be updated: https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/README.md
Please make the follow up PR.

@srimanob
Copy link
Contributor

+Upgrade

This PR adds new Phase-2 mkfit workflow, and it will run in the long matrix (IB). The new workflow runs fine in the PR test.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @rappoccio, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)
Notice This PR was tested with additional Pull Request(s), please also merge them if necessary: cms-data/RecoTracker-MkFit#14

@mmasciov
Copy link
Contributor Author

Hi @mmasciov @slava77 @mmusich The readme should also be updated: https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/README.md Please make the follow up PR.

Thanks, @srimanob!
Done in #44839.

@slava77
Copy link
Contributor

slava77 commented Apr 24, 2024

@cms-sw/orp-l2 @rappoccio
please comment if this can make it to pre3.

only your "ORP" signature is missing

@antoniovilela
Copy link
Contributor

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 0b7b45a into cms-sw:master Apr 24, 2024
12 checks passed
cmsbuild added a commit that referenced this pull request Apr 24, 2024
Add description of .702 workflow offset, introduced in PR #44708
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