customize L1FPGATrackProducer

tschuh commented 2 years ago

This PR does:

adds set function in Settings for removalType and doMultipleMatches
extends L1FPGATrackProducer to manipulate Settings::removalType and Settings::doMultipleMatches via python config
adapts HybridTracksNewKF_cfg.py, L1TrackNtupleMaker_cfg.py and demonstrator_cfg.py to use this function
removes obsolete L1FPGATrackProducer clones from Tracklet_cfi.py and L1HybridEmulationTracks_cff.py
adapts input tag in ProducerKF_cfi.py to not point to obsolete L1FPGATrackProducer clones This PR depends on https://github.com/cms-L1TK/cmssw/pull/111

This PR introduces a duplication of some cfg params, as explained in https://github.com/cms-L1TK/cmssw/issues/134 . This should ultimately be eliminated, as suggested in the "Issue". But the PR can be approved without doing this.

tschuh commented 2 years ago

@tomalin could you tell me why the gui reports failing CI while the CI reports https://gitlab.cern.ch/cms-l1tk/cmssw_CI/-/pipelines/3337331/builds all test passed?

tomalin commented 2 years ago

Please rebase this branch, to make it easier to understand which code must be reviewed.

tschuh commented 2 years ago

Please rebase this branch, to make it easier to understand which code must be reviewed.

done

tomalin commented 2 years ago

As you've introduced the reducedConfig() function is Customize_cff.py, do we still need the TTTracksFromReducedTrackletEmulation section of Tracklet_cfi.py?

tomalin commented 2 years ago

At https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/python/ChannelAssignment_cfi.py#L11 , there's a typo in the comment "sedd type". Also the parameter here MaxNumProjectionLayers isn't use anywhere. And it seems unlikely that a seed could ever project to 8 additional layers.

tschuh commented 2 years ago

As you've introduced the reducedConfig() function is Customize_cff.py, do we still need the TTTracksFromReducedTrackletEmulation section of Tracklet_cfi.py?

As long as TTTracksFromReducedTrackletEmulation is used you need it, however one could use everywhere the reducedConfig instead. Currently people have the choice of what they prefer.

tschuh commented 2 years ago

At https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/python/ChannelAssignment_cfi.py#L11 , there's a typo in the comment "sedd type". Also the parameter here MaxNumProjectionLayers isn't use anywhere. And it seems unlikely that a seed could ever project to 8 additional layers.

it is used in two places atm and becomes obsolete in tschuh_kfin. Seed types L1L2 and L2L3 to project to 8 layers as you can see specified in the same file.

tomalin commented 2 years ago

Add comment to start of HybridTracksNewKF_cfg.py explaining what it is for. i.e. Why one wouldn't just run L1TrackNtupleMaker_cfg.py.

tomalin commented 2 years ago

Clarify comment explaining IRChannelsIn at https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/python/ChannelAssignment_cfi.py#L33 . e.g. "ID of DTC connected to each IR". I assume the DTC ID is that returned by Setup::dtcId() https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackTrigger/interface/Setup.h#L63 . And I guess the IR ID is obtained from the order in which the IR is listed in the processingmodules.dat file? If so, this needs stating too.

tomalin commented 2 years ago

Is this comment accurate https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/interface/ChannelAssignment.h#L15 ? It says this class assigns both Stubs & Tracks to Tracklet output channel. But whereas I see a function "bool channelId(const TTTrackRef& ttTrackRef, int& channelId) " to assign Tracks to channel, I don't see an equivalent function for Stubs. Nor to I see an equivalent function to assign Stubs to the Tracklet input channel, which the comment also says this class does. Though I do see one that "assigns DTC output channel to Tracklet input channel".

tomalin commented 2 years ago

I've finished reviewing this PR, if @tschuh can answer the remaining comments.

I've asked @aryd for advice on my suggestion above to eliminate cfg param IRChannelsIn". If we can determine this automatically, it is preferable to forcing the user to set these numbers by hand for every new HW test setup.

I've asked Stefano Mersi to tell us if each DTC crate contains 3 or 4 DTC reading 10 Gb/s PS modules.

tschuh commented 2 years ago

Add comment to start of HybridTracksNewKF_cfg.py explaining what it is for. i.e. Why one wouldn't just run L1TrackNtupleMaker_cfg.py.

done

tschuh commented 2 years ago

Clarify comment explaining IRChannelsIn at https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/python/ChannelAssignment_cfi.py#L33 . e.g. "ID of DTC connected to each IR". I assume the DTC ID is that returned by Setup::dtcId() https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackTrigger/interface/Setup.h#L63 . And I guess the IR ID is obtained from the order in which the IR is listed in the processingmodules.dat file? If so, this needs stating too.

done

tschuh commented 2 years ago

Is this comment accurate https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/interface/ChannelAssignment.h#L15 ? It says this class assigns both Stubs & Tracks to Tracklet output channel. But whereas I see a function "bool channelId(const TTTrackRef& ttTrackRef, int& channelId) " to assign Tracks to channel, I don't see an equivalent function for Stubs. Nor to I see an equivalent function to assign Stubs to the Tracklet input channel, which the comment also says this class does. Though I do see one that "assigns DTC output channel to Tracklet input channel".

Yes it is, I the function returning stub channel ID is called layerId

tomalin commented 2 years ago

Can we agree that it's desirable to eliminate IRChannelsIn from https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/python/Customize_cff.py#L26 , instead taking this info from the tracklet wiring map? And that we should find a way (e.g. making the wiring map an ESProduct) of doing this? If so, I suggest adding just above L26 a comment to remind us.

"TO DO: Eliminate cfg param IRChannelsIn by taking this info from Tracklet wiring map."

And adding a git "Issue", with containing:

"TO DO: Eliminate cfg param IRChannelsIn from from https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/python/Customize_cff.py#L26 by taking this info from Tracklet wiring map. e.g. The list above "0, 1, 25," indicates that 2nd IR module (where counting starts at 0) is connected to DTC ID = 25. And LUTsReduced/processingmodules.dat says that2nd IR is named IR_PS10G_2_B , which wires.dat says is connected to the DTC named DL_PS10G_2_B . And using array dtcname (from L1FPGATrackProducer.cc), one finds that "PS10G_2" is in slot 1 of the crate, and this name also means its at +ve z and in the 2nd Tracker sector connected to this TFP region. From this info, a function added to your Setup class should be able to return the DTC ID. The difficulty is that EDProducer ChannelAssignment accesses IRChannelsIn, but doesn't have access to the wiring map."

tomalin commented 2 years ago

As you've introduced the reducedConfig() function is Customize_cff.py, do we still need the TTTracksFromReducedTrackletEmulation section of Tracklet_cfi.py?

As long as TTTracksFromReducedTrackletEmulation is used you need it, however one could use everywhere the reducedConfig instead. Currently people have the choice of what they prefer.

If Customize_cff.py contains function reducedConfig() and Tracklet_cfi.py contains TTTracksFromReducedTrackletEmulation, then we have duplicate python code specifying the summer chain. I'd therefore like to delete TTTracksFromReducedTrackletEmulation from Tracklet_cfi.py, which I suspect requires modifying https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker_cfg.py#L175 to use Customize_cff.py

tomalin commented 2 years ago

@tschuh If you can address remaining comments above, and ensure we don't forget the one open question by following the advice in https://github.com/cms-L1TK/cmssw/pull/112#issuecomment-1040549418 , I could approve this PR.

tschuh commented 2 years ago

Can we agree that it's desirable to eliminate IRChannelsIn from https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/python/Customize_cff.py#L26 , instead taking this info from the tracklet wiring map? And that we should find a way (e.g. making the wiring map an ESProduct) of doing this? If so, I suggest adding just above L26 a comment to remind us.

"TO DO: Eliminate cfg param IRChannelsIn by taking this info from Tracklet wiring map."

And adding a git "Issue", with containing:

"TO DO: Eliminate cfg param IRChannelsIn from from https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/python/Customize_cff.py#L26 by taking this info from Tracklet wiring map. e.g. The list above "0, 1, 25," indicates that 2nd IR module (where counting starts at 0) is connected to DTC ID = 25. And LUTsReduced/processingmodules.dat says that2nd IR is named IR_PS10G_2_B , which wires.dat says is connected to the DTC named DL_PS10G_2_B . And using array dtcname (from L1FPGATrackProducer.cc), one finds that "PS10G_2" is in slot 1 of the crate, and this name also means its at +ve z and in the 2nd Tracker sector connected to this TFP region. From this info, a function added to your Setup class should be able to return the DTC ID. The difficulty is that EDProducer ChannelAssignment accesses IRChannelsIn, but doesn't have access to the wiring map."

done

tschuh commented 2 years ago

As you've introduced the reducedConfig() function is Customize_cff.py, do we still need the TTTracksFromReducedTrackletEmulation section of Tracklet_cfi.py?

As long as TTTracksFromReducedTrackletEmulation is used you need it, however one could use everywhere the reducedConfig instead. Currently people have the choice of what they prefer.

If Customize_cff.py contains function reducedConfig() and Tracklet_cfi.py contains TTTracksFromReducedTrackletEmulation, then we have duplicate python code specifying the summer chain. I'd therefore like to delete TTTracksFromReducedTrackletEmulation from Tracklet_cfi.py, which I suspect requires modifying https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/test/L1TrackNtupleMaker_cfg.py#L175 to use Customize_cff.py

done

tschuh commented 2 years ago

@tschuh If you can address remaining comments above, and ensure we don't forget the one open question by following the advice in #112 (comment) , I could approve this PR.

done, please let me know if I missed something

tomalin commented 2 years ago

Is this comment accurate https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/interface/ChannelAssignment.h#L15 ? It says this class assigns both Stubs & Tracks to Tracklet output channel. But whereas I see a function "bool channelId(const TTTrackRef& ttTrackRef, int& channelId) " to assign Tracks to channel, I don't see an equivalent function for Stubs. Nor to I see an equivalent function to assign Stubs to the Tracklet input channel, which the comment also says this class does. Though I do see one that "assigns DTC output channel to Tracklet input channel".

Yes it is, I the function returning stub channel ID is called layerId

Ah, I see. For the TrackBuilder output, the number returned by ChannelAssignment::layerId() is both an indication of layer number and of channel no.. Please clarify the comment for function layerId() to make this clear. This will relate it more obviously to the comment at the top of the class about the class assigning stubs to channels.

tschuh commented 2 years ago

Is this comment accurate https://github.com/cms-L1TK/cmssw/blob/tschuh_customize/L1Trigger/TrackFindingTracklet/interface/ChannelAssignment.h#L15 ? It says this class assigns both Stubs & Tracks to Tracklet output channel. But whereas I see a function "bool channelId(const TTTrackRef& ttTrackRef, int& channelId) " to assign Tracks to channel, I don't see an equivalent function for Stubs. Nor to I see an equivalent function to assign Stubs to the Tracklet input channel, which the comment also says this class does. Though I do see one that "assigns DTC output channel to Tracklet input channel".

Yes it is, I the function returning stub channel ID is called layerId

Ah, I see. For the TrackBuilder output, the number returned by ChannelAssignment::layerId() is both an indication of layer number and of channel no.. Please clarify the comment for function layerId() to make this clear. This will relate it more obviously to the comment at the top of the class about the class assigning stubs to channels.

done

cms-L1TK / cmssw

customize L1FPGATrackProducer #112