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

Fix cluster->TP matching for pixel tracking CPU workflow #111

Merged

Conversation

makortel
Copy link

@makortel makortel commented Aug 1, 2018

This detail got overlooked in #105.

@fwyzard @VinInn

@makortel makortel mentioned this pull request Aug 1, 2018
@VinInn
Copy link

VinInn commented Aug 1, 2018

~gpu ????
is it really python syntax?? (of course it is, still...)

@makortel
Copy link
Author

makortel commented Aug 1, 2018

Yes, ~ is the binary negation. It has been used to negate the results of EDFilters for a long time (see e.g. the example config of #100), and recently I introduced it for the modifiers to be able to handle certain complex cases. (I agree that here it is really an abuse, and the tpCluster config should be organized differently, but I leave that to the post-#100 time).

@@ -716,6 +716,7 @@ def _uniqueFirstLayers(layerList):
tpClusterProducerPixelTrackingOnly = tpClusterProducer.clone()
# Need to use the modifier to customize because the exact EDProducer type depends on the modifier
gpu.toModify(tpClusterProducerPixelTrackingOnly, src = "tpClusterProducerHeterogeneousPixelTrackingOnly")
(~gpu).toModify(tpClusterProducerPixelTrackingOnly, pixelClusterSrc = "siPixelClustersPreSplitting")
Copy link

Choose a reason for hiding this comment

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

mhm, this is above the level of ugliness that I would like to have...

can we set it to siPixelClustersPreSplitting by default, and change it to something else for the gpu case ?

@makortel
Copy link
Author

makortel commented Aug 1, 2018

Better now? (basically moving the gpu.toReplaceWith() down to TrackValidation_cff).

@fwyzard
Copy link

fwyzard commented Aug 1, 2018

I guess... though it took me few minutes to understand the changes.

The reason you couldn't do tpClusterProducerPixelTrackingOnly = tpClusterProducer.clone( pixelClusterSrc = "siPixelClustersPreSplitting" ) in the first place, is that tpClusterProducerPixelTrackingOnly is an EDProducer of a different type depending whether gpu is in effect or not, and the two types use a different parameter (src vs pixelClusterSrc) ?

@fwyzard
Copy link

fwyzard commented Aug 1, 2018

So, users of tpClusterProducer should actually use tpClusterProducer in the CPU case, and tpClusterProducerHeterogeneous + tpClusterProducerConverter in the GPU case ?

@makortel
Copy link
Author

makortel commented Aug 1, 2018

The reason you couldn't do tpClusterProducerPixelTrackingOnly = tpClusterProducer.clone( pixelClusterSrc = "siPixelClustersPreSplitting" ) in the first place, is that tpClusterProducerPixelTrackingOnly is an EDProducer of a different type depending whether gpu is in effect or not, and the two types use a different parameter (src vs pixelClusterSrc) ?

Yes, exactly. This is the nasty side effect of applying toReplaceWith to modules.

(in #100 everybody is forced to clone/modify the CPU/GPU/whatever flavor modules explicitly)

@makortel
Copy link
Author

makortel commented Aug 1, 2018

So, users of tpClusterProducer should actually use tpClusterProducer in the CPU case, and tpClusterProducerHeterogeneous + tpClusterProducerConverter in the GPU case ?

Yes.

Even though if this model would be fully deployed, there would be only tpClusterProducerHeterogeneous + tpClusterProducerConverter (and the former runs the CPU algorithm in absence of a GPU).

@fwyzard
Copy link

fwyzard commented Aug 1, 2018

Validation summary

Reference release CMSSW_10_2_0_pre6 at a674e1f
Development branch CMSSW_10_2_X_Patatrack at d2ca336
Testing PRs:

makeTrackValidationPlots.py plots

/RelValTTbar_13/CMSSW_10_2_0_pre6-PU25ns_102X_upgrade2018_realistic_v7-v1/GEN-SIM-DIGI-RAW

/RelValZMM_13/CMSSW_10_2_0_pre6-102X_upgrade2018_realistic_v7-v1/GEN-SIM-DIGI-RAW

DQM GUI plots

/RelValTTbar_13/CMSSW_10_2_0_pre6-PU25ns_102X_upgrade2018_realistic_v7-v1/GEN-SIM-DIGI-RAW

/RelValZMM_13/CMSSW_10_2_0_pre6-102X_upgrade2018_realistic_v7-v1/GEN-SIM-DIGI-RAW

logs and nvprof/nvvp profiles

/RelValTTbar_13/CMSSW_10_2_0_pre6-PU25ns_102X_upgrade2018_realistic_v7-v1/GEN-SIM-DIGI-RAW

/RelValZMM_13/CMSSW_10_2_0_pre6-102X_upgrade2018_realistic_v7-v1/GEN-SIM-DIGI-RAW

Logs

The full log is available at https://fwyzard.web.cern.ch/fwyzard/patatrack/pulls/a5cc8af2966d4af03256e3fb24ad72c24acd3493/log .

@fwyzard
Copy link

fwyzard commented Aug 1, 2018

Fixes 10824.5 and 10824.7 .

@fwyzard fwyzard merged commit 9090a44 into cms-patatrack:CMSSW_10_2_X_Patatrack Aug 1, 2018
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.

3 participants