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

Customize premixing stage1 and parts of stage2 with modifiers, and extend Modifier boolean expressions to include "not" and "or" #22210

Merged
merged 17 commits into from Feb 26, 2018

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Feb 14, 2018

Currently premixing customizations are implemented as separate files that import or copy from the standard ones, and get used in the configurations according to the cmsDriver parameters. This PR replaces the customizations in stage1 fully, and in stage2 partially, with "process modifier". This is a preparatory step for phase2 premixing developments (making phase2 customizations a bit easier to do). This PR also addresses #21704.

In a bit more detail this PR

  • adds modifiers premix_stage1 and premix_stage2
    • --procModifiers parameter is added to all premixing workflows
  • removes DIGIPREMIX and DIGIPREMIX_S2 steps from cmsDriver (both can be now just DIGI)
  • migrates (i.e. removes) configuration files
    • stage1: Digi_Premix_cff, DigiToRawPremixing_cff, SimL1EmulatorPremix_cff, digi_noNoise_cfi and "DIGIPREMIX terrible hacks"
    • stage2: digi_MixPreMix_cfi, DigiDMPremix_cff

What remains for stage2 is at least DataMixerPreMix_cff. It wasn't clear to me whether moving the customizations to the default DataMixerDataOnSim_cff would actually be an improvement, so I decided to leave that for a later exercise (and then one would have to think the relationship of --procModifiers premix_stage2 and --datamix cmsDriver parameters).

The idea has been discussed in #21704 and in simulation meeting on Feb 6th.
https://indico.cern.ch/event/702324/contributions/2885358/attachments/1596021/2527902/slides_mk_simulation_20180206.pdf

As a sidestep, in order to avoid adding isChosen() calls, the Modifier boolean expressions are extended with "not" (~) and "or" (|). For completeness added support for toReplaceWith() to the resulting Modifier of the boolean expressions.

Tested in CMSSW_10_1_X_2018-02-11-2300, no changes expected in results. I've tested with workflows 250199.{0,17,18} and 250202.{0,17,18} (such that the 250202.X use premixing library from 250202.X as an input) that the DQM histograms agree, and that the configuration dumps (from both stage1 and stage2) do not contain essential differences (there are many changes visible not affecting any functionality because the "customization point" is moved earlier in the chain of cloning of PSets).

@mdhildreth @kpedro88

_enableDigiAliases hack is no longer needed

Remove Digi_PreMix_cff as unnecessary
…zation

Note that HCAL phase2 packing+unpacking was enabled in cms-sw#20920, so
there is no need to remove that for phase2 premixing (which anyway is
not being tested or working at the moment).

Also note that in L1TDigiToRaw_cff we have to reset the
rawDataCollector.RawCollectionList as if the L1TDigiToRaw_cff would
not have been loaded. This was not needed earlier because the file was
explicitly not loaded.
Fully replaced with premix_stage1 modifier
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

Configuration/Applications
Configuration/ProcessModifiers
Configuration/PyReleaseValidation
Configuration/StandardSequences
EventFilter/ESDigiToRaw
EventFilter/SiStripRawToDigi
FastSimulation/Configuration
L1Trigger/Configuration
SimCalorimetry/Configuration
SimCalorimetry/EcalSelectiveReadoutProducers
SimCalorimetry/EcalSimProducers
SimCalorimetry/HcalSimProducers
SimCalorimetry/HcalZeroSuppressionProducers
SimGeneral/MixingModule
SimGeneral/PileupInformation
SimMuon/RPCDigitizer
SimTracker/SiPhase2Digitizer

@perrotta, @cmsbuild, @prebello, @lveldere, @emeschi, @civanch, @mdhildreth, @fabozzi, @nsmith-, @rekovic, @franzoni, @kpedro88, @thomreis, @slava77, @mommsen, @GurpreetSinghChahal, @fabiocos, @ssekmen, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @kreczko, @felicepantaleo, @sdevissc, @GiacomoSguazzoni, @jhgoh, @VinInn, @Martin-Grunewald, @ebrondol, @rovere, @matt-komm, @mariadalfonso, @argiro, @threus, @dgulhan this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

@cmsbuild, please test with 250199.0,250199.17,250199.18,250202.0,250202.17,250202.18

(even if these tests do not provide any evidence that the stage1 workflows 250199.X would still produce the same output)

@fabozzi
Copy link
Contributor

fabozzi commented Feb 23, 2018

+1

@ssekmen
Copy link
Contributor

ssekmen commented Feb 23, 2018

+1

@civanch
Copy link
Contributor

civanch commented Feb 24, 2018

+1

@fabiocos
Copy link
Contributor

+operations

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

@Dr15Jones for my understanding, could you please clarify why is the failing unit test expected to do so?

@makortel
Copy link
Contributor Author

@fabiocos May I ask if you're waiting for @Dr15Jones' reply before merging? (I guess the fully signed + orp signature does not lead to automatic merge because of a failing unit test)

@Dr15Jones
Copy link
Contributor

testPythonParameterSet problem was discussed in this hypernews threads
https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3398/1/1.html

essentially, if you run the tests in FWCore/PythonParameterSet but have not checked out FWCore/ParameterSet into your local are, this happens.

@fabiocos
Copy link
Contributor

@Dr15Jones ok, thanks for the clarification

@fabiocos
Copy link
Contributor

merge

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