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

Add Support for View of vector of Ptr #8559

Merged
merged 86 commits into from Apr 4, 2015

Conversation

wddgit
Copy link
Contributor

@wddgit wddgit commented Mar 26, 2015

Add support for View of vector of Ptr. This completes implementation of this new feature. There is a test of its usage in the class ViewAnalyzer in function testStdVectorPtr which is run by ViewTest_cfg.py in the package FWCore/Integration. This does change interfaces. Code in the release was fixed to compile and so that runTheMatrix will pass. But runtime problems could exist in code not run in runThe Matrix and compile time problems for code not in the release. For example, it is no longer possible to get the container product from a Ref which implies many things. For example on cannot construct a RefProd from a Ref. One change in particular that might cause problems. If one inserts into an AssociationMap constructed with the no argument constructor an exception will be thrown at runtime, There is known code like this in the release, although none is called in runTheMatrix tests.

Dr15Jones and others added 30 commits December 9, 2014 23:22
By allowing Ptr and RefToBase to be used to lookup items with an
AssociationVector we can avoid an unnecessary creation of a Ref.
Use C++11 perfect forwarding to handle constructing of key and value
components from allowed constructor arguments. This makes creating an
AssociationMap easier.
It is now a requirement that AssociationMap be initialized with information
about which container(s) it will use.
…lookup

Changed AssocationMap so that it is possible to lookup items use a
Ptr or a RefToBase. This avoids requiring users to create a edm::Ref
just for lookup.
Added a constructor to AssociationMap to allow one to associate which
collection(s) the container will use for association. This constructor
must be used since it is no longer possible to have those collections
be set on the first insert to the container.
The design which allowed a edm::RefProd to be constructed from an
edm::Ref caused problems for future changes (edm::View and slimming).
Given this was equivalent to taking an arbitrary pointer and then
obtaining which std::vector<> the data is stored in, we determined
this was not an absolute requirement.
The design which allowed a RefToBaseProd to be constructed from an
edm::Ref or RefToBase caused problems for future changes (edm::View
and slimming). Given this was equivalent to taking an arbitrary pointer
and then obtaining which std::vector<> the data is stored in, we
determined this was not an absolute requirement.
Views no longer give access to ProductID or DataProductGetter
so one can no longer construct a RefToBaseProd from a View.
In order to support reading an std::vector<edm::Ptr<>> into a
View it was necessary to remove the use of edm::PtrVector and
edm::RefToBaseVector from View.
This also required the ability to create edm::RefToBase<> objects
out of edm::Ptr instances.
To avoid unnecessary clutter in header files and to allow faster compilation for the case where the specialization isn't needed, the specializations of HolderToVectorTrait were moved to their own files.
The use of the other header's include guards to guarantee we only include the code when the other two files are included is unorthodox but works in this case since all files live in the same package.
In order to support reading a std::vector<edm::Ptr<>> into a View, we need to record additional information.
In addition, the code no longer needs to use RefHolder based classes.
This change sets up all the code needed to switch on reading an std::vector<edm::Ptr<T>> by an edm::View<T>.
In addition to the test addition, the code was also made to conform to the new threaded framework.
-The edm:RefToBase<>::castTo was being used as a complicated and slow
way of doing a dynamic cast of the element of the container.
-The optional call to getByToken for std::vector<Trajectory> was using
the same GetByTokenT<> as for the Tracks. This can not work since they
are different types. Changed to use a separate container of tokens for
the trajectories.
-Removed unnecessary copies of containers from the Event.
-Used a reference to the Track directly rather than making a unnecessary
TrackRef.
The code was using edm::RefToBase<>::castTo as a complicated and
slow method of doing a dynamic_cast of the element of the container.
Doing the dynamic_cast directly is cleaner and faster as well as
avoids future complications with castTo.
By allowing Ptr and RefToBase to be used to lookup items with an
AssociationVector we can avoid an unnecessary creation of a Ref.
Use C++11 perfect forwarding to handle constructing of key and value
components from allowed constructor arguments. This makes creating an
AssociationMap easier.
It is now a requirement that AssociationMap be initialized with information
about which container(s) it will use.
…lookup

Changed AssocationMap so that it is possible to lookup items use a
Ptr or a RefToBase. This avoids requiring users to create a edm::Ref
just for lookup.
Added a constructor to AssociationMap to allow one to associate which
collection(s) the container will use for association. This constructor
must be used since it is no longer possible to have those collections
be set on the first insert to the container.
The design which allowed a edm::RefProd to be constructed from an
edm::Ref caused problems for future changes (edm::View and slimming).
Given this was equivalent to taking an arbitrary pointer and then
obtaining which std::vector<> the data is stored in, we determined
this was not an absolute requirement.
The design which allowed a RefToBaseProd to be constructed from an
edm::Ref or RefToBase caused problems for future changes (edm::View
and slimming). Given this was equivalent to taking an arbitrary pointer
and then obtaining which std::vector<> the data is stored in, we
determined this was not an absolute requirement.
Makes the pointer cached in a Ref point to the
element in the container instead of pointing
to the container. Removes the product function
from classes related to Ref, RefToBase, Ptr, and
vectors of them. Remove constructor of Ref from
RefVector. Add Ref constructor that takes ProductID,
pointer to element, and a key. Remove RefProd
constructor from Ref and RefProd constructor from
RefVector. Remove  RefToBaseProd constructors from
Ref, RefToBase, and View. Make RefToBaseProd from
RefProd constructor explicit.

This is all motivated by changes to the View class
that indirectly require this to work properly.

Many changes to make code that depends on these changes
continue work as before.

Additions to make it easier to build AssociationMap
with a one argument constructor.

A previous commit required the AssociationMap be
initialized with RefProd before calling insert. This
commit fixes some but not all of the cases where
this caused failures.

Note this is work in progress. If you try to run with
this commit failures will occur.
MasterCollection now needs the Event to make the
RefToBaseProd in MasterCollection.
@slava77
Copy link
Contributor

slava77 commented Apr 1, 2015

+1

for #8559 36f7f53
code changes are in line with expectations at visual inspection.

jenkins tests of RECO workflows pass OK and show no differences in monitored quantities (fwlite or DQM based comparisons).

@wddgit
Copy link
Contributor Author

wddgit commented Apr 1, 2015

Thanks Slava. Can I suggest other people go ahead and review/approve this? I was hoping for the jenkins tests to pass, but the currently running ones seem stuck and the previous 6 tries all failed for different reasons unrelated to this PR. This one was the best: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8559/3874/summary.html. Only an unrelated llvm compiler warning and the PR to fix that was independently submitted already. (Core and DQM already approved except I put in a change to fix an unrelated unit test failure after that)

@slava77
Copy link
Contributor

slava77 commented Apr 2, 2015

I echo David's request.
Changes are primarily technical.

@davidlange6
Copy link
Contributor

All - i propose to merge this Saturday unless there are complaints in the meanwhile.

davidlange6 added a commit that referenced this pull request Apr 4, 2015
Add Support for View of vector of Ptr
@davidlange6 davidlange6 merged commit 9adf9e7 into cms-sw:CMSSW_7_5_X Apr 4, 2015
@wddgit wddgit deleted the viewWithVectorOfPtr4 branch May 19, 2015 19:45
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

10 participants