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

Hydjet Interface External #28764

Closed
wants to merge 7 commits into from
Closed

Hydjet Interface External #28764

wants to merge 7 commits into from

Conversation

wouf
Copy link
Contributor

@wouf wouf commented Jan 20, 2020

PR description:

Hydjet inerface for using as external with cms-sw/cmsdist#5412

PR validation:

This is not final version, we are trying to solve #28636

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28764/13414

  • This PR adds an extra 32KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28764/13418

  • This PR adds an extra 32KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28764/13433

  • This PR adds an extra 32KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 23, 2020

The tests are being triggered in jenkins.
Tested with other pull request(s) cms-sw/cmsdist#5412
Test Parameters:

@cmsbuild
Copy link
Contributor

+1
Tested at: a6eede4
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-927a64/4329/summary.html
CMSSW: CMSSW_11_1_X_2020-01-23-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-927a64/4329/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2697090
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2696743
  • DQMHistoTests: Total skipped: 346
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@silviodonato
Copy link
Contributor

@@ -32,7 +36,7 @@ namespace HepMC {
namespace gen {
class Pythia6Service;

class HydjetHadronizer : public BaseHadronizer {
class HydjetHadronizer : public BaseHadronizer, public edm::EDConsumerBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not work as intended. The framework will not magically get the consumes() information by arbitrary components declaring to inherit from edm::EDConsumerBase. The edm::ConsumesCollector should be used instead to call consumes() (it needs to be percolated to the constructor of HydjectHadronizer from the constructors of all the EDModules that use it).

Copy link
Contributor Author

@wouf wouf Jan 28, 2020

Choose a reason for hiding this comment

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

@makortel I assumed to use e.setConsumer(this) after getEDMEvent() or setEDMEvent() (which we have to add somewhere). In tests it looks worked, but I can be wrong...

Copy link
Contributor

Choose a reason for hiding this comment

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

That is not sufficient (anyway Event::setConsumer() mostly framework internal function). Framework needs to know the consumes information (=data dependencies) at the module construction time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@makortel Isn't it?
Sorry if I'm being stupid, but could You please to explain Your suggestion to replace edm::EDConsumerBase by edm::ConsumesCollector? I mean how to implement it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it?

No. The call to the EDConsumerBase constructor is far from sufficient. In short the framework asks from the EDConsumerBase objects it knows (=EDModules constructed by the framework) which products are consumed. The framework is not aware of arbitrary classes that decide to inherit from EDConsumerBase.

Sorry if I'm being stupid, but could You please to explain Your suggestion to replace edm::EDConsumerBase by edm::ConsumesCollector? I mean how to implement it?

See https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideEDMGetDataFromEvent#Consumes_and_Helpers

Essentially it should be something along the EDModule using HydjectHadronizer, e.g. edm::GeneratorFilter<HAD, DEC> doing

  template <class HAD, class DEC>
  GeneratorFilter<HAD, DEC>::GeneratorFilter(ParameterSet const& ps)
      : EDFilter(), hadronizer_(ps, consumesCollector()), decayer_(nullptr), nEventsInLumiBlock_(0) {

and then HydjectHadronizer doing

HydjetHadronizer::HydjetHadronizer(const ParameterSet& pset, ConsumesCollector iC)
... {
  ...
  if (embedding_) {
    cflag_ = 0;
    src_ = iC.consumes<HepMCProduct>(
        pset.getUntrackedParameter<edm::InputTag>("backgroundLabel", edm::InputTag("generator", "unsmeared")));
  }
  ...
}

And yes, it does imply that all hadronizers that are used in conjunction with edm::GeneratorFilter<...> need to be modified to take edm::ConsumesCollector argument (that they can ignore).

Copy link
Contributor Author

@wouf wouf Jan 31, 2020

Choose a reason for hiding this comment

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

@makortel Thanks! It works, but with an exception:

Exception Message:
Unrunnable schedule
Module run order problem found: 
generatorSmeared after VtxSmeared [path generation_step], VtxSmeared after mix [path generation_step], mix after randomEngineStateProducer [path generation_step], randomEngineStateProducer after generator [path generation_step], generator consumes generatorSmeared
 Running in the threaded framework would lead to indeterminate results.
 Please change order of modules in mentioned Path(s) to avoid inconsistent module ordering.

I think we have firstly to solve the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

It works,

Good.

I think we have firstly to solve the issue.

Correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't it?

No. The call to the EDConsumerBase constructor is far from sufficient. In short the framework asks from the EDConsumerBase objects it knows (=EDModules constructed by the framework) which products are consumed. The framework is not aware of arbitrary classes that decide to inherit from EDConsumerBase.

Sorry if I'm being stupid, but could You please to explain Your suggestion to replace edm::EDConsumerBase by edm::ConsumesCollector? I mean how to implement it?

See https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideEDMGetDataFromEvent#Consumes_and_Helpers

Essentially it should be something along the EDModule using HydjectHadronizer, e.g. edm::GeneratorFilter<HAD, DEC> doing

  template <class HAD, class DEC>
  GeneratorFilter<HAD, DEC>::GeneratorFilter(ParameterSet const& ps)
      : EDFilter(), hadronizer_(ps, consumesCollector()), decayer_(nullptr), nEventsInLumiBlock_(0) {

I changed all generators for this, but have a trouble with Pythia8:

'''

Compiling edm plugin /afs/cern.ch/work/w/wouf/HYDJET/1.9.1/EmbeddingProj/CMSSW_11_1_0_pre5/src/GeneratorInterface/Pythia8Interface/plugins/Pythia8Hadronizer.cc
In file included from /cvmfs/cms.cern.ch/slc7_amd64_gcc820/external/dire/2.004-bcolbf/include/Dire/SplitInfo.h:17,
from /cvmfs/cms.cern.ch/slc7_amd64_gcc820/external/dire/2.004-bcolbf/include/Dire/Splittings.h:15,
from /cvmfs/cms.cern.ch/slc7_amd64_gcc820/external/dire/2.004-bcolbf/include/Dire/SplittingLibrary.h:15,
from /cvmfs/cms.cern.ch/slc7_amd64_gcc820/external/dire/2.004-bcolbf/include/Dire/Dire.h:8,
from /afs/cern.ch/work/w/wouf/HYDJET/1.9.1/EmbeddingProj/CMSSW_11_1_0_pre5/src/GeneratorInterface/Pythia8Interface/plugins/Pythia8Hadronizer.cc:15:
/cvmfs/cms.cern.ch/slc7_amd64_gcc820/external/dire/2.004-bcolbf/include/Dire/DireBasics.h:108:7: warning: 'class Pythia8::DireFunction' has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor]
class DireFunction {
^~~~~~~~~~~~
In file included from /afs/cern.ch/work/w/wouf/HYDJET/1.9.1/EmbeddingProj/CMSSW_11_1_0_pre5/src/GeneratorInterface/Pythia8Interface/plugins/Pythia8Hadronizer.cc:61:
/afs/cern.ch/work/w/wouf/HYDJET/1.9.1/EmbeddingProj/CMSSW_11_1_0_pre5/src/GeneratorInterface/Core/interface/ConcurrentHadronizerFilter.h: In instantiation of 'edm::gen::StreamCache<HAD, DEC>::StreamCache(const edm::ParameterSet&) [with HAD = Pythia8Hadronizer; DEC = gen::ConcurrentExternalDecayDriver]':
/cvmfs/cms.cern.ch/slc7_amd64_gcc820/external/gcc/8.2.0-pafccj/include/c++/8.3.1/bits/unique_ptr.h:831:30: required from 'typename std::_MakeUniq<_Tp>::__single_object std::make_unique(_Args&& ...) [with _Tp = edm::gen::StreamCache<Pythia8Hadronizer, gen::ConcurrentExternalDecayDriver>; _Args = {const edm::ParameterSet&}; typename std::_MakeUniq<_Tp>::_single_object = std::unique_ptr<edm::gen::StreamCache<Pythia8Hadronizer, gen::ConcurrentExternalDecayDriver>, std::default_delete<edm::gen::StreamCache<Pythia8Hadronizer, gen::ConcurrentExternalDecayDriver> > >]'
/afs/cern.ch/work/w/wouf/HYDJET/1.9.1/EmbeddingProj/CMSSW_11_1_0_pre5/src/GeneratorInterface/Core/interface/ConcurrentHadronizerFilter.h:176:62: required from 'std::unique_ptr<edm::gen::StreamCache<HAD, DEC> > edm::ConcurrentHadronizerFilter<HAD, DEC>::beginStream(edm::StreamID) const [with HAD = Pythia8Hadronizer; DEC = gen::ConcurrentExternalDecayDriver]'
/afs/cern.ch/work/w/wouf/HYDJET/1.9.1/EmbeddingProj/CMSSW_11_1_0_pre5/src/GeneratorInterface/Core/interface/ConcurrentHadronizerFilter.h:175:47: required from here
/afs/cern.ch/work/w/wouf/HYDJET/1.9.1/EmbeddingProj/CMSSW_11_1_0_pre5/src/GeneratorInterface/Core/interface/ConcurrentHadronizerFilter.h:70:65: error: no matching function for call to 'Pythia8Hadronizer::Pythia8Hadronizer()'
StreamCache(ParameterSet const& iPSet) : hadronizer
{iPSet} {}
^
/afs/cern.ch/work/w/wouf/HYDJET/1.9.1/EmbeddingProj/CMSSW_11_1_0_pre5/src/GeneratorInterface/Pythia8Interface/plugins/Pythia8Hadronizer.cc:170:1: note: candidate: 'Pythia8Hadronizer::Pythia8Hadronizer(const edm::ParameterSet&, edm::ConsumesCollector&&)'
Pythia8Hadronizer::Pythia8Hadronizer(const edm::ParameterSet &params, edm::ConsumesCollector&& iC)
^~~~~~~~~~~~~~~~~
/afs/cern.ch/work/w/wouf/HYDJET/1.9.1/EmbeddingProj/CMSSW_11_1_0_pre5/src/GeneratorInterface/Pythia8Interface/plugins/Pythia8Hadronizer.cc:170:1: note: candidate expects 2 arguments, 1 provided
'''

@makortel @mkirsano Could You please help to fix it?

src_ = pset.getParameter<edm::InputTag>("backgroundLabel");
if (embedding_) {
cflag_ = 0;
src_ = mayConsume<HepMCProduct>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why mayConsume()? Below the HepMCProduct is read always when embedding_ is true, so this could be just consumes().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks.

if (embedding_) {
cflag_ = 0;
src_ = mayConsume<HepMCProduct>(
pset.getUntrackedParameter<edm::InputTag>("backgroundLabel", edm::InputTag("generator", "unsmeared")));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this parameter stay as tracked? It affects physics and therefore should to be included in the event provenance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@makortel It does not included to the gen fragment by default, so to avoid changing all configs I added default value. Anyway for current underlying sample it can't be various. In case of different label (then underlying sample has) the generation fails.

@silviodonato
Copy link
Contributor

This PR is related to the issue #28636

@silviodonato
Copy link
Contributor

@wouf after the long discussion on #28636, it seems you have all pieces to run hydjet as externals. Are you going to update this PR or do you prefer to open a new one?

@wouf
Copy link
Contributor Author

wouf commented May 27, 2020

@wouf after the long discussion on #28636, it seems you have all pieces to run hydjet as externals.

Not really, at this moment we are trying to solve issue with OscarMTProducer warnings:

%MSG-w SimG4CoreGenerator: in event :  OscarMTProducer:g4SimHits  23-Apr-2020 09:05:14 CEST Run: 1 Event: 10
10 a particle  pdgid= 2101 has status=2 but has no decay vertex, so will be fully tracked by Geant4
%MSG

which is coming from PYTHIA6's incorrect childes information in some cases. In that cases PYJETS common block contains:

#           1  PDG:        2212  st:        21  mother:           0  childes:            0 -           0
#           2  PDG:        2112  st:        21  mother:           0  childes:            0 -           0
#           3  PDG:          21  st:          21  mother:           1  childes:            0 -           0
#           4  PDG:          21  st:          21  mother:           2  childes:            0 -           0
#           5  PDG:          21  st:          21  mother:           3  childes:            0 -           0
#           6  PDG:          21  st:          21  mother:           4  childes:            0 -           0 
#           7  PDG:          21  st:          21  mother:           0  childes:            0 -           0 
#           8  PDG:          21  st:          21  mother:          0  childes:            0 -           0 
#           9  PDG:           2  st:          13  mother:           7  childes:    200100000 -           0
#          10  PDG:          21  st:          13  mother:           0  childes:    200110000 -   200090000
#          11  PDG:          21  st:          13  mother:           8  childes:    200120000 -   200100000
#          12  PDG:          21  st:          13  mother:           8  childes:    200130000 -   200110000

At this moment we are trying to work around.

Are you going to update this PR or do you prefer to open a new one?

I think the new one would be better.

@silviodonato
Copy link
Contributor

I think the new one would be better.
Let me close this PR then. If/when you want to reopen it, just let me know and I will do it.

@wouf
Copy link
Contributor Author

wouf commented Jun 10, 2020

Thank You @silviodonato , all!
The new PR in #30185

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

6 participants