[RFC] Replace RangeMap used in Muon hit storage #42917
Conversation
The hit classes are not polymorphic so OwnVector was unnecessary.
…nVector Can read files containing data products with RangeMap with OwnVectors using these modules which read the old class and generate the new class.
Test that comparator used with RangeMap properly functions.
The type is not polymorphic so an std::vector is a better choice.
The classes changed - have hits which were not being stored polymorphically - were written to either standard AOD or MiniAOD The IdToHitRange is easier for ROOT to store as it does not use an std::map internally.
|
For further documentation, the two non-standard comparators used with the DTSuperLayerIdComparator CSCDetIdSameChamberComparator I didn't write a unit test for CSCDetIdSameChamberComparator as I could confirm it was a degenerate case via inspection. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42917/37058
|
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages:
@tvami, @mandrenguyen, @makortel, @civanch, @AdrianoDee, @smuzaffar, @cmsbuild, @srimanob, @jfernan2, @mdhildreth, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
I have opened the PR to allow broader testing as well as to start the discussion about such a migration (with further discussions also the appropriate meetings). |
|
please test |
|
-1 Failed Tests: UnitTests RelVals RelVals-INPUT AddOn Unit TestsI found 14 errors in the following unit tests: ---> test DiMuonVertex had ERRORS ---> test runtestPhysicsToolsPatAlgos had ERRORS ---> test runtestUtilAlgos had ERRORS and more ... RelValsRelVals-INPUT
Expand to see more relval errors ...
AddOn Tests |
| <version ClassVersion="10" checksum="3541168201"/> | ||
| <version ClassVersion="11" checksum="841063529"/> |
There was a problem hiding this comment.
By quick look I'd think the MuonSegment's DTRecSegment4DRef and CSCSegmentRef members would be broken. The types should point to the new container types, but the Refs ProductID would point to the data products read from the input file.
There was a problem hiding this comment.
Ah, I see now that ROOT's schema evolution outright fails for the Ref members.
| private: | ||
| // ---------- member data -------------------------------- | ||
| edm::EDGetTokenT<edm::RangeMap<ID, edm::OwnVector<T>>> get_; | ||
| edm::EDPutTokenT<edm::RangeMap<ID, std::vector<T>>> put_; |
There was a problem hiding this comment.
Should this class template really convert from RangeMap<ID, OwnVector<T>> to IdToHitRange<ID, T>? Or am I missing something?
| template <typename CMP> | ||
| range get(ID id, CMP comparator) const { | ||
| if (ids_.size() != offsets_.size()) { | ||
| throw edm::Exception(edm::errors::LogicError, "calling get with comparitor before sorting."); |
There was a problem hiding this comment.
| throw edm::Exception(edm::errors::LogicError, "calling get with comparitor before sorting."); | |
| throw edm::Exception(edm::errors::LogicError, "calling get with comparator before sorting."); |
?
| std::vector<ID> ids_; | ||
| std::vector<unsigned int> offsets_; | ||
| container collection_; |
There was a problem hiding this comment.
Could you add some comments how the ID element gets mapped to collection_ via offsets_?
There was a problem hiding this comment.
Is the idea that, previously map<ID,pair<unsigned int,unsigned int> > was storing the start and stop indices, and since the container is contiguous (i.e. start[i+1] = stop[i]), the offsets is sufficient?
| @@ -65,7 +65,8 @@ initial version number of a class which has never been stored before. | |||
| <version ClassVersion="11" checksum="541727491"/> | |||
| <version ClassVersion="10" checksum="4050071853"/> | |||
| </class> | |||
| <class name="reco::MuonSegmentMatch" ClassVersion="11"> | |||
| <class name="reco::MuonSegmentMatch" ClassVersion="12"> | |||
| <version ClassVersion="12" checksum="3329152288"/> | |||
There was a problem hiding this comment.
I'd expect the MuonSegmentMatch member muon hit Refs to be broken here as well.
| @@ -83,7 +83,7 @@ namespace edm { | |||
| "edm::AssociationMap<edm::OneToManyWithQuality<(.*?), *(.*?), *(.*?), *u[a-z]*> >"); | |||
| static std::regex const reToVector("edm::AssociationVector<(.*), *(.*), *edm::Ref.*,.*>"); | |||
| //NOTE: if the item within a clone policy is a template, this substitution will probably fail | |||
| static std::regex const reToRangeMap("edm::RangeMap< *(.*), *(.*), *edm::ClonePolicy<([^>]*)> >"); | |||
| static std::regex const reToRangeMap("edm::RangeMap< *(.*), *(.*), *edm::(Clone|Copy)Policy<([^>]*)> >"); | |||
There was a problem hiding this comment.
Given that the strategy change towards edm::IdToHitRange, are these changes needed?
|
why not to use edmnew::DetSetVector (that was implemented just for that)? the copy (to ease transition) is implemented here |
|
Milestone for this pull request has been moved to CMSSW_14_1_X. Please open a backport if it should also go in to CMSSW_14_0_X. |
|
Milestone for this pull request has been moved to CMSSW_14_2_X. Please open a backport if it should also go in to CMSSW_14_1_X. |
|
ping (to make bot change milestone) |
PR description:
Changed the use of
edm::RangeMap<ID, edm::OwnVector<HIT>>in muon code toedm::IdToHitRange<ID, HIT>. This is a part of the migration away from the use ofedm::OwnVector(see #42734 ). The classes modified are those which are stored to either RECO, AOD, or MiniAOD files and theHITclass does not have any inheriting classes so the use of anedm::OwnVectorwas never needed. Theedm::IdToHitRangeshould take up less memory as well as be easier/faster/smaller to store (measurements need to be done to confirm this).As part of the change, new EDProducers were introduced to be able to convert
edm::RangeMapinto the equivalentedm::IdToHitRange. Thes EDProducers can be used to allow reading of old files containingedm::RangeMapand then covert to the new type as needed.The
edm::IdToHitRangeis designed to be a drop-in replacement toedm::RangeMapso shares the same API. One of the functions inedm::RangMapallows one to pass in a different comparison function in order to find a range of hits. This is a very dangerous item as it requires the new comparison to be degenerate with respect to the standard comparator used to sort inedm::RangeMap. I added a new unit test for one of the two comparators to prove that it meets that standard (and to guarantee if it is ever change to keep meeting that standard).PR validation:
Code compiles and new unit tests pass. I ran workflow 12434.0 using the new code.