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

[RFC] Draft of PR to CMSSW #15

Closed
wants to merge 18 commits into from
Closed

[RFC] Draft of PR to CMSSW #15

wants to merge 18 commits into from

Conversation

makortel
Copy link
Collaborator

@makortel makortel commented May 6, 2021

No description provided.

@slava77
Copy link

slava77 commented May 6, 2021

how does this differ from what's already in CMSSW_11_2_0_mkFit_X?

'Reco',
'RecoGlobal',
],
PU = [],
suffix = '_trackingMkFit',
offset = 0.7,
)
upgradeWFs['trackingMkFit'].step2 = {
'--customise': 'RecoTracker/MkFit/customizeHLTIter0ToMkFit.customizeHLTIter0ToMkFit'
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we want to include a customize function for the HLT iter0? (there is a risk for future maintenance effort given that it would be part of a runTheMatrix workflow)

RecoTracker/MkFit/plugins/MkFitGeometryESProducer.cc Outdated Show resolved Hide resolved
@makortel
Copy link
Collaborator Author

makortel commented May 6, 2021

how does this differ from what's already in CMSSW_11_2_0_mkFit_X?

  • References to detachedTripletStep, mixedTripletStep, pixelLessStep, and tobTecStep are removed
  • RecoTracker/MkFit/python/input.py and RecoTracker/MkFit/test/reco_cfg.py are removed
  • Few fixes to customizeHLTIter0ToMkFit #16 is included
  • Rebased on top of CMSSW_11_3_0_pre6

@slava77
Copy link

slava77 commented May 6, 2021

References to detachedTripletStep, mixedTripletStep, pixelLessStep, and tobTecStep are removed

I'm not sure this is expected.
I was expecting that we port everything and just not add these iterations at the trackingMkFit = cms.ModifierChain(

@makortel
Copy link
Collaborator Author

makortel commented May 6, 2021

References to detachedTripletStep, mixedTripletStep, pixelLessStep, and tobTecStep are removed

I'm not sure this is expected.
I was expecting that we port everything and just not add these iterations at the trackingMkFit = cms.ModifierChain(

I wasn't clear on last Friday then :( I find it a bit more clear to provide customizations only for those iterations that we actually enable with trackingMkFit ModifierChain.

@slava77
Copy link

slava77 commented May 6, 2021

I wasn't clear on last Friday then :( I find it a bit more clear to provide customizations only for those iterations that we actually enable with trackingMkFit ModifierChain.

IMHO, this will make the overhead for testing these "not yet enabled" iterations too high.
Instead of being able to test essentially directly on top of CMSSW, we'll need out private PRs

BTW, the more recent version with cleaning by shared hit enabled already looks like pixelLessStep and tobTecStep can be enabled.

@makortel
Copy link
Collaborator Author

makortel commented May 6, 2021

I wasn't clear on last Friday then :( I find it a bit more clear to provide customizations only for those iterations that we actually enable with trackingMkFit ModifierChain.

IMHO, this will make the overhead for testing these "not yet enabled" iterations too high.
Instead of being able to test essentially directly on top of CMSSW, we'll need out private PRs

Fair point, although I had imagined that we'd anyway use a custom branch on top of CMSSW, and that our development would continue in 11_2_0. If the private RecoTracker/MkFit/test/reco_cfg.py is not that useful and people want to work on the bleeding edge of CMSSW (which has its own benefits), I can certainly add the customizations back to the "omitted" iterations.

@makortel
Copy link
Collaborator Author

The last two force-pushes include

  • added back customizations for detachedTriplet, mixedTriplet, pixelLess, tobTec
    • detachedTriplet, mixedTriplet stay disabled by default
    • pixelLess and tobTec are enabled by default
  • included Simplify includes #17

@slava77
Copy link

slava77 commented May 12, 2021

should we have a modifier and config for initialStepPreSplitting? I suppose that it can be the same as initialStep

@makortel
Copy link
Collaborator Author

should we have a modifier and config for initialStepPreSplitting? I suppose that it can be the same as initialStep

Eventually yes. If this is wanted for Monday, I might be able to add it on Friday. I suppose we'd need new MTV plots if we enable it by default, I won't have time to do that.

@slava77
Copy link

slava77 commented May 12, 2021

should we have a modifier and config for initialStepPreSplitting? I suppose that it can be the same as initialStep

Eventually yes. If this is wanted for Monday, I might be able to add it on Friday. I suppose we'd need new MTV plots if we enable it by default, I won't have time to do that.

I think that it should be OK to have this for a follow up later.
It's one of the most trivial items to reduce the total CPU time and it perhaps has the least impact on physics even if mkfit behavior is suboptimal.

cerati and others added 3 commits May 19, 2021 12:34
Original names were CMS-2017-HitSelectionWindows.h, CMS-2017.acc, CMS-2017.cc. Changes include
- Rename files to adhere CMS' naming conventions
- Adhere CMS' naming conventions
- Code format
- Replace global std::function object with a function
- Call createCMS2017 instead of ExecTrackerInfoCreatorPlugin
- Add comment on writes to global variables
@makortel
Copy link
Collaborator Author

This PR has served its purpose

@makortel makortel closed this May 21, 2021
mmasciov pushed a commit that referenced this pull request Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants