Merge pull request cms-sw#901 from Dr15Jones/productIDBug

Framework buggix -- Fixed bug between edm::Refs and EDAlias

DataFormats/Provenance/interface/BranchIDListHelper.h

@@ -21,7 +21,7 @@ namespace edm {
     void fixBranchListIndexes(BranchListIndexes& indexes);
 
     BranchIDLists const& branchIDLists() const {return branchIDLists_;}
-    BranchIDLists& branchIDLists() {return branchIDLists_;}
+    BranchIDLists& mutableBranchIDLists() {return branchIDLists_;}
     BranchIDToIndexMap const& branchIDToIndexMap() const {return branchIDToIndexMap_;}
 
   private:

DataFormats/Provenance/src/BranchIDListHelper.cc

@@ -12,6 +12,10 @@ namespace edm {
 
   bool
   BranchIDListHelper:: updateFromInput(BranchIDLists const& bidlists) {
+    //The BranchIDLists is a list of lists
+    // this routine compares bidlists to branchIDLists_ to see if a list
+    // in branchIDLists_ is already in bidlist and if it isn't we insert
+    // that new list into branchIDLists_
     bool unchanged = true;
     branchListIndexMapper_.clear();
     typedef BranchIDLists::const_iterator Iter;

FWCore/Framework/interface/OutputModule.h

@@ -116,10 +116,6 @@ namespace edm {
       return droppedBranchIDToKeptBranchID_;
     }
 
-    std::map<BranchID::value_type, BranchID::value_type> const& keptBranchIDToDroppedBranchID() {
-      return keptBranchIDToDroppedBranchID_;
-    }
-
   private:
 
     int maxEvents_;
@@ -157,7 +153,6 @@ namespace edm {
     // needed because of possible EDAliases.
     // filled in only if key and value are different.
     std::map<BranchID::value_type, BranchID::value_type> droppedBranchIDToKeptBranchID_;
-    std::map<BranchID::value_type, BranchID::value_type> keptBranchIDToDroppedBranchID_;
     std::unique_ptr<BranchIDLists> branchIDLists_;
     BranchIDLists const* origBranchIDLists_;
 

FWCore/Framework/interface/one/OutputModuleBase.h

@@ -130,10 +130,6 @@ namespace edm {
         return droppedBranchIDToKeptBranchID_;
       }
 
-      std::map<BranchID::value_type, BranchID::value_type> const& keptBranchIDToDroppedBranchID() {
-        return keptBranchIDToDroppedBranchID_;
-      }
-
     private:
 
       int maxEvents_;
@@ -171,7 +167,6 @@ namespace edm {
       // needed because of possible EDAliases.
       // filled in only if key and value are different.
       std::map<BranchID::value_type, BranchID::value_type> droppedBranchIDToKeptBranchID_;
-      std::map<BranchID::value_type, BranchID::value_type> keptBranchIDToDroppedBranchID_;
       std::unique_ptr<BranchIDLists> branchIDLists_;
       BranchIDLists const* origBranchIDLists_;
 

FWCore/Framework/src/OutputModule.cc

@@ -37,7 +37,6 @@ namespace edm {
     selectors_(),
     selector_config_id_(),
     droppedBranchIDToKeptBranchID_(),
-    keptBranchIDToDroppedBranchID_(),
     branchIDLists_(new BranchIDLists),
     origBranchIDLists_(nullptr),
     branchParents_(),
@@ -147,7 +146,6 @@ namespace edm {
         if(keptBranchID != branchID) {
           // An EDAlias branch was persisted.
           droppedBranchIDToKeptBranchID_.insert(std::make_pair(branchID.id(), keptBranchID.id()));
-          keptBranchIDToDroppedBranchID_.insert(std::make_pair(keptBranchID.id(), branchID.id()));
         }
       }
     }
@@ -302,11 +300,6 @@ namespace edm {
       // Check for branches dropped while an EDAlias was kept.
       for(BranchIDList& branchIDList : *branchIDLists_) {
         for(BranchID::value_type& branchID : branchIDList) {
-          // Replace BranchID of each kept alias branch with zero, so only the product ID of the original branch will be accessible.
-          std::map<BranchID::value_type, BranchID::value_type>::const_iterator kiter = keptBranchIDToDroppedBranchID_.find(branchID);
-          if(kiter != keptBranchIDToDroppedBranchID_.end()) {
-            branchID = 0;
-          }
           // Replace BranchID of each dropped branch with that of the kept alias, so the alias branch will have the product ID of the original branch.
           std::map<BranchID::value_type, BranchID::value_type>::const_iterator iter = droppedBranchIDToKeptBranchID_.find(branchID);
           if(iter != droppedBranchIDToKeptBranchID_.end()) {

FWCore/Framework/src/SubProcess.cc

@@ -259,7 +259,7 @@ namespace edm {
   SubProcess::fixBranchIDListsForEDAliases(std::map<BranchID::value_type, BranchID::value_type> const& droppedBranchIDToKeptBranchID) {
     // Check for branches dropped while an EDAlias was kept.
     // Replace BranchID of each dropped branch with that of the kept alias.
-    for(BranchIDList& branchIDList : branchIDListHelper_->branchIDLists()) {
+    for(BranchIDList& branchIDList : branchIDListHelper_->mutableBranchIDLists()) {
       for(BranchID::value_type& branchID : branchIDList) {
         std::map<BranchID::value_type, BranchID::value_type>::const_iterator iter = droppedBranchIDToKeptBranchID.find(branchID);
         if(iter != droppedBranchIDToKeptBranchID.end()) {

FWCore/Framework/src/one/OutputModuleBase.cc

@@ -49,7 +49,6 @@ namespace edm {
     selectors_(),
     selector_config_id_(),
     droppedBranchIDToKeptBranchID_(),
-    keptBranchIDToDroppedBranchID_(),
     branchIDLists_(new BranchIDLists),
     origBranchIDLists_(nullptr),
     branchParents_(),
@@ -159,7 +158,6 @@ namespace edm {
           if(keptBranchID != branchID) {
             // An EDAlias branch was persisted.
             droppedBranchIDToKeptBranchID_.insert(std::make_pair(branchID.id(), keptBranchID.id()));
-            keptBranchIDToDroppedBranchID_.insert(std::make_pair(keptBranchID.id(), branchID.id()));
           }
         }
       }
@@ -288,11 +286,6 @@ namespace edm {
         // Check for branches dropped while an EDAlias was kept.
         for(BranchIDList& branchIDList : *branchIDLists_) {
           for(BranchID::value_type& branchID : branchIDList) {
-            // Replace BranchID of each kept alias branch with zero, so only the product ID of the original branch will be accessible.
-            std::map<BranchID::value_type, BranchID::value_type>::const_iterator kiter = keptBranchIDToDroppedBranchID_.find(branchID);
-            if(kiter != keptBranchIDToDroppedBranchID_.end()) {
-              branchID = 0;
-            }
             // Replace BranchID of each dropped branch with that of the kept alias, so the alias branch will have the product ID of the original branch.
             std::map<BranchID::value_type, BranchID::value_type>::const_iterator iter = droppedBranchIDToKeptBranchID_.find(branchID);
             if(iter != droppedBranchIDToKeptBranchID_.end()) {

FWCore/Integration/test/OtherThingAlgorithm.cc

@@ -5,19 +5,13 @@
 #include "DataFormats/Common/interface/RefToPtr.h"
 
 namespace edmtest {
-  void OtherThingAlgorithm::run(edm::Event const& event, 
+  void OtherThingAlgorithm::run(edm::Handle<ThingCollection> const& parentHandle,
 				OtherThingCollection& result,
-        edm::EDGetToken token,
 				bool useRefs,
 				bool refsAreTransient) {
 
     const size_t numToMake = 20;
     result.reserve(numToMake);
-    edm::Handle<ThingCollection> parentHandle;
-    if(useRefs) {
-      assert(event.getByToken(token, parentHandle));
-      assert(parentHandle.isValid());
-    }
     ThingCollection const* parent = parentHandle.product();
     ThingCollection const* null = 0;
 

FWCore/Integration/test/OtherThingAlgorithm.h

@@ -3,6 +3,7 @@
 
 #include <string>
 #include "DataFormats/TestObjects/interface/OtherThingCollectionfwd.h"
+#include "DataFormats/TestObjects/interface/ThingCollectionfwd.h"
 #include "FWCore/Framework/interface/Frameworkfwd.h"
 #include "FWCore/Utilities/interface/EDGetToken.h"
 
@@ -14,9 +15,8 @@ namespace edmtest {
 
     /// Runs the algorithm and returns a list of OtherThings
     /// The user declares the vector and calls this method.
-    void run(edm::Event const& ev, 
+    void run(edm::Handle<ThingCollection> const& iThingHandle,
              OtherThingCollection& otherThingCollection,
-             edm::EDGetToken token,
              bool useRefs = true,
              bool refsAreTransient = false);
 

FWCore/Integration/test/OtherThingProducer.cc

@@ -1,10 +1,35 @@
-#include "FWCore/Integration/test/OtherThingProducer.h"
+#include <string>
+
 #include "DataFormats/TestObjects/interface/OtherThingCollection.h"
 #include "FWCore/Framework/interface/Event.h"
 #include "FWCore/Framework/interface/MakerMacros.h"
+#include "FWCore/Framework/interface/Frameworkfwd.h"
+#include "FWCore/Framework/interface/EDProducer.h"
+
+#include "FWCore/Integration/test/OtherThingAlgorithm.h"
+#include "FWCore/Utilities/interface/EDGetToken.h"
 #include "FWCore/Utilities/interface/InputTag.h"
 
+
 namespace edmtest {
+  class OtherThingProducer : public edm::EDProducer {
+  public:
+    explicit OtherThingProducer(edm::ParameterSet const& ps);
+
+    virtual ~OtherThingProducer();
+
+    virtual void produce(edm::Event& e, edm::EventSetup const& c);
+
+    static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);
+
+  private:
+    OtherThingAlgorithm alg_;
+    edm::EDGetToken thingToken_;
+    bool useRefs_;
+    bool refsAreTransient_;
+  };
+
+
   OtherThingProducer::OtherThingProducer(edm::ParameterSet const& pset): alg_(), refsAreTransient_(false) {
     produces<OtherThingCollection>("testUserTag");
     useRefs_ = pset.getUntrackedParameter<bool>("useRefs");
@@ -24,10 +49,19 @@ namespace edmtest {
     // Step B: Create empty output 
     std::auto_ptr<OtherThingCollection> result(new OtherThingCollection);  //Empty
 
-    // Step C: Invoke the algorithm, passing in inputs (NONE) and getting back outputs.
-    alg_.run(e, *result, thingToken_, useRefs_, refsAreTransient_);
+    // Step C: Get data for algorithm
+    edm::Handle<ThingCollection> parentHandle;
+    if(useRefs_) {
+      bool succeeded = e.getByToken(thingToken_, parentHandle);
+      assert(succeeded);
+      assert(parentHandle.isValid());
+    }
+
+
+    // Step D: Invoke the algorithm, passing in inputs (NONE) and getting back outputs.
+    alg_.run(parentHandle, *result, useRefs_, refsAreTransient_);
 
-    // Step D: Put outputs into event
+    // Step E: Put outputs into event
     e.put(result, std::string("testUserTag"));
   }
 

IOPool/Input/test/PoolAliasTestStep1_DifferentOrder_cfg.py

@@ -0,0 +1,43 @@
+# The following comments couldn't be translated into the new config version:
+
+# Configuration file for PrePoolInputTest 
+
+import FWCore.ParameterSet.Config as cms
+
+process = cms.Process("TESTPROD")
+process.load("FWCore.Framework.test.cmsExceptionsFatal_cff")
+
+process.maxEvents = cms.untracked.PSet(
+    input = cms.untracked.int32(11)
+)
+process.Thing = cms.EDProducer("ThingProducer")
+
+process.output = cms.OutputModule("PoolOutputModule",
+    outputCommands = cms.untracked.vstring("keep *", "drop *_Thing_*_TESTPROD"),
+    fileName = cms.untracked.string('aliastest_step1_diffOrder.root')
+)
+
+process.source = cms.Source("EmptySource",
+    firstLuminosityBlock = cms.untracked.uint32(6),
+    numberEventsInLuminosityBlock = cms.untracked.uint32(3),
+    firstRun = cms.untracked.uint32(561),
+    numberEventsInRun = cms.untracked.uint32(7)
+)
+
+process.OtherThing = cms.EDProducer("OtherThingProducer",
+    thingTag = cms.InputTag("ZThing")
+    )
+
+#This changes the order of the original and alias branch names and branchIDs
+process.ZThing = cms.EDAlias(
+    Thing = cms.VPSet(
+      cms.PSet(type = cms.string('edmtestThings'),
+               fromProductInstance = cms.string('*'),
+               toProductInstance = cms.string('*'))
+    )
+)
+
+
+process.p = cms.Path(process.Thing*process.OtherThing)
+process.ep = cms.EndPath(process.output)
+

IOPool/Input/test/PoolAliasTestStep2_DifferentOrder_cfg.py

@@ -0,0 +1,31 @@
+# The following comments couldn't be translated into the new config version:
+
+# Configuration file for PoolInputTest
+
+import FWCore.ParameterSet.Config as cms
+
+process = cms.Process("TESTANALYSIS")
+process.load("FWCore.Framework.test.cmsExceptionsFatal_cff")
+
+process.maxEvents = cms.untracked.PSet(
+    input = cms.untracked.int32(-1)
+)
+
+process.Analysis = cms.EDAnalyzer("OtherThingAnalyzer")
+
+process.source = cms.Source("PoolSource",
+    setRunNumber = cms.untracked.uint32(621),
+    fileNames = cms.untracked.vstring('file:aliastest_step1_diffOrder.root')
+)
+
+process.p = cms.Path(process.Analysis)
+
+#Also test creating a Ref from the alias branch
+process.OtherThing2 = cms.EDProducer("OtherThingProducer",
+                                    thingTag = cms.InputTag("ZThing")
+                                    )
+
+process.Analysis2 = cms.EDAnalyzer("OtherThingAnalyzer",
+                                   other=cms.untracked.InputTag("OtherThing2","testUserTag"))
+
+process.p2 = cms.Path(process.OtherThing2+process.Analysis2)

IOPool/Input/test/PoolAliasTestStep2_cfg.py

@@ -20,4 +20,12 @@
 
 process.p = cms.Path(process.Analysis)
 
+#Also test creating a Ref from the alias branch
+process.OtherThing2 = cms.EDProducer("OtherThingProducer",
+                                    thingTag = cms.InputTag("AltThing")
+                                    )
 
+process.Analysis2 = cms.EDAnalyzer("OtherThingAnalyzer",
+                                   other=cms.untracked.InputTag("OtherThing2","testUserTag"))
+
+process.p2 = cms.Path(process.OtherThing2+process.Analysis2)

IOPool/Input/test/TestPoolInput.sh

@@ -43,6 +43,10 @@ cmsRun --parameter-set ${LOCAL_TEST_DIR}/PoolAliasTestStep1_cfg.py || die 'Failu
 
 cmsRun --parameter-set ${LOCAL_TEST_DIR}/PoolAliasTestStep2_cfg.py || die 'Failure using PoolAliasTestStep2_cfg.py' $?
 
+cmsRun --parameter-set ${LOCAL_TEST_DIR}/PoolAliasTestStep1_DifferentOrder_cfg.py || die 'Failure using PoolAliasTestStep1_DifferentOrder_cfg.py' $?
+
+cmsRun --parameter-set ${LOCAL_TEST_DIR}/PoolAliasTestStep2_DifferentOrder_cfg.py || die 'Failure using PoolAliasTestStep2_DifferentOrder_cfg.py' $?
+
 cmsRun --parameter-set ${LOCAL_TEST_DIR}/PoolAliasTestStep2A_cfg.py || die 'Failure using PoolAliasTestStep2A_cfg.py' $?
 
 cmsRun --parameter-set ${LOCAL_TEST_DIR}/PoolAliasTestStep1C_cfg.py || die 'Failure using PoolAliasTestStep2A_cfg.py' $?