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

[RFC] Replace RangeMap used in Muon hit storage #42917

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion AnalysisDataFormats/SUSYBSMObjects/src/classes_def.xml
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@
<class name="edm::Wrapper<susybsm::HSCPDeDxInfoCollection>"/>
<class name="edm::Wrapper<susybsm::HSCPDeDxInfoValueMap>"/>

<class name="susybsm::MuonSegment" ClassVersion="10">
<class name="susybsm::MuonSegment" ClassVersion="11">
<version ClassVersion="10" checksum="3541168201"/>
<version ClassVersion="11" checksum="841063529"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see now that ROOT's schema evolution outright fails for the Ref members.

</class>
<class name="susybsm::MuonSegmentCollection"/>
<class name="susybsm::MuonSegmentRef"/>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
#ifndef CommonTools_RecoAlgos_RangeMapOwnVectorToVectorConverter_h
#define CommonTools_RecoAlgos_RangeMapOwnVectorToVectorConverter_h
// -*- C++ -*-
//
// Package: CommonTools/RecoAlgos
// Class : RangeMapOwnVectorToVectorConverter
//
/**\class RangeMapOwnVectorToVectorConverter RangeMapOwnVectorToVectorConverter.h "CommonTools/RecoAlgos/interface/RangeMapOwnVectorToVectorConverter.h"

Description: [one line class summary]

Usage:
<usage>

*/
//
// Original Author: Christopher Jones
// Created: Mon, 25 Sep 2023 14:10:44 GMT
//

// system include files
#include <vector>

// user include files
#include "DataFormats/Common/interface/RangeMap.h"
#include "DataFormats/Common/interface/OwnVector.h"
#include "FWCore/Framework/interface/global/EDProducer.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"
#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
#include "FWCore/Utilities/interface/InputTag.h"
#include "FWCore/Utilities/interface/EDGetToken.h"
#include "FWCore/Utilities/interface/EDPutToken.h"

// forward declarations

namespace reco {
template <typename ID, typename T>
class RangeMapOwnVectorToVectorConverter : public edm::global::EDProducer<> {
public:
RangeMapOwnVectorToVectorConverter(edm::ParameterSet const& iPSet)
: get_(consumes(iPSet.getParameter<edm::InputTag>("get"))), put_(produces()) {}

RangeMapOwnVectorToVectorConverter(const RangeMapOwnVectorToVectorConverter&) = delete; // stop default
const RangeMapOwnVectorToVectorConverter& operator=(const RangeMapOwnVectorToVectorConverter&) =
delete; // stop default

// ---------- const member functions ---------------------
void produce(edm::StreamID, edm::Event& iEvent, edm::EventSetup const&) const final {
auto ownRange = iEvent.get(get_);

edm::RangeMap<ID, std::vector<T>> vecRange;
for (auto ids = ownRange.id_begin(); ids != ownRange.id_end(); ++ids) {
auto range = ownRange.get(*ids);
vecRange.put(*ids, range.first, range.second);
}
iEvent.emplace(put_, std::move(vecRange));
}

static void fillDescriptions(edm::ConfigurationDescriptions& iConfig) {
edm::ParameterSetDescription pset;
pset.add<edm::InputTag>("get");

iConfig.addDefault(pset);
}

private:
// ---------- member data --------------------------------
edm::EDGetTokenT<edm::RangeMap<ID, edm::OwnVector<T>>> get_;
edm::EDPutTokenT<edm::RangeMap<ID, std::vector<T>>> put_;
Copy link
Contributor

@makortel makortel Sep 29, 2023

Choose a reason for hiding this comment

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

Should this class template really convert from RangeMap<ID, OwnVector<T>> to IdToHitRange<ID, T>? Or am I missing something?

};
} // namespace reco
#endif
12 changes: 6 additions & 6 deletions DataFormats/CSCRecHit/interface/CSCRecHit2DCollection.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@
* The collection of CSCRecHit2D's. See \ref CSCRecHit2DCollection.h for details.
*
*/
#include <DataFormats/MuonDetId/interface/CSCDetId.h>
#include <DataFormats/CSCRecHit/interface/CSCRecHit2D.h>
#include <vector>

#include <DataFormats/Common/interface/RangeMap.h>
#include <DataFormats/Common/interface/ClonePolicy.h>
#include <DataFormats/Common/interface/OwnVector.h>
#include "DataFormats/MuonDetId/interface/CSCDetId.h"
#include "DataFormats/CSCRecHit/interface/CSCRecHit2D.h"

typedef edm::RangeMap<CSCDetId, edm::OwnVector<CSCRecHit2D> > CSCRecHit2DCollection;
#include "DataFormats/Common/interface/IdToHitRange.h"

using CSCRecHit2DCollection = edm::IdToHitRange<CSCDetId, CSCRecHit2D>;

#endif
14 changes: 6 additions & 8 deletions DataFormats/CSCRecHit/interface/CSCSegmentCollection.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,15 @@
* \author Matteo Sani
*/

#include <DataFormats/MuonDetId/interface/CSCDetId.h>
#include <DataFormats/CSCRecHit/interface/CSCSegment.h>
#include "DataFormats/MuonDetId/interface/CSCDetId.h"
#include "DataFormats/CSCRecHit/interface/CSCSegment.h"

#include <DataFormats/Common/interface/RangeMap.h>
#include <DataFormats/Common/interface/ClonePolicy.h>
#include <DataFormats/Common/interface/OwnVector.h>
#include "DataFormats/Common/interface/IdToHitRange.h"

typedef edm::RangeMap<CSCDetId, edm::OwnVector<CSCSegment> > CSCSegmentCollection;
using CSCSegmentCollection = edm::IdToHitRange<CSCDetId, CSCSegment>;

#include <DataFormats/Common/interface/Ref.h>
typedef edm::Ref<CSCSegmentCollection> CSCSegmentRef;
#include "DataFormats/Common/interface/Ref.h"
using CSCSegmentRef = edm::Ref<CSCSegmentCollection>;

//typedef std::vector<CSCSegment> CSCSegmentCollection;

Expand Down
4 changes: 4 additions & 0 deletions DataFormats/CSCRecHit/src/classes_def.xml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
<class name="edm::ClonePolicy<CSCRecHit2D>"/> <!-- Root6 -->
<class name="edm::RangeMap<CSCDetId,edm::OwnVector<CSCRecHit2D,edm::ClonePolicy<CSCRecHit2D> >,edm::ClonePolicy<CSCRecHit2D> >"/>
<class name="edm::Wrapper<edm::RangeMap<CSCDetId,edm::OwnVector<CSCRecHit2D,edm::ClonePolicy<CSCRecHit2D> >,edm::ClonePolicy<CSCRecHit2D> > >"/>
<class name="edm::IdToHitRange<CSCDetId,CSCRecHit2D>"/>
<class name="edm::Wrapper<edm::IdToHitRange<CSCDetId,CSCRecHit2D>>"/>

<class name="std::vector<CSCSegment>"/>
<class name="std::vector<CSCSegment*>"/>
Expand All @@ -26,8 +28,10 @@
</class>
<class name="edm::OwnVector<CSCSegment,edm::ClonePolicy<CSCSegment> >"/>
<class name="edm::RangeMap<CSCDetId,edm::OwnVector<CSCSegment,edm::ClonePolicy<CSCSegment> >,edm::ClonePolicy<CSCSegment> >"/>
<class name="edm::IdToHitRange<CSCDetId,CSCSegment>"/>
<class name="edm::ClonePolicy<CSCSegment>"/> <!-- Root6 -->
<class name="edm::Wrapper<edm::RangeMap<CSCDetId,edm::OwnVector<CSCSegment,edm::ClonePolicy<CSCSegment> >,edm::ClonePolicy<CSCSegment> > >"/>
<class name="edm::Wrapper<edm::IdToHitRange<CSCDetId,CSCSegment>>"/>
<class name="CSCSegmentRef"/>
</selection>

Expand Down
220 changes: 220 additions & 0 deletions DataFormats/Common/interface/IdToHitRange.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,220 @@
#ifndef DataFormats_Common_IdToHitRange_h
#define DataFormats_Common_IdToHitRange_h
// -*- C++ -*-
//
// Package: DataFormats/Common
// Class : IdToHitRange
//
/**\class IdToHitRange IdToHitRange.h "DataFormats/Common/interface/IdToHitRange.h"

Description: Used to associate an ID to a range of hits

Usage:
Allows quick lookup of a range of hits via the ID

*/
//
// Original Author: Christopher Jones
// Created: Tue, 26 Sep 2023 20:41:14 GMT
//

// system include files
#include <vector>
#include <algorithm>

// user include files
#include "DataFormats/Common/interface/CMS_CLASS_VERSION.h"
#include "FWCore/Utilities/interface/EDMException.h"

// forward declarations

namespace edm {
template <typename ID, typename HIT>
class IdToHitRange {
public:
using container = std::vector<HIT>;
/// contained object type
using value_type = typename container::value_type;
/// collection size type
using size_type = typename container::size_type;
/// reference type
using reference = typename container::reference;
/// pointer type
using pointer = typename container::pointer;
/// constant access iterator type
using const_iterator = typename container::const_iterator;
/// iterator range
using range = std::pair<const_iterator, const_iterator>;

using id_iterator = typename std::vector<ID>::const_iterator;

public:
IdToHitRange() {}

// ---------- const member functions ---------------------
/// get a range of objects with specified identifier
range get(ID id) const {
auto i = find(id);
if (i == ids_.end()) {
return {collection_.end(), collection_.end()};
}
auto offsetInOffsets = i - ids_.begin();
if (ids_.size() != offsets_.size()) {
offsetInOffsets = offsets_[offsetInOffsets] + ids_.size();
}
auto startIndex = offsets_[offsetInOffsets];
if (static_cast<std::size_t>(offsetInOffsets + 1) == offsets_.size()) {
return {collection_.begin() + startIndex, collection_.end()};
}
auto endIndex = offsets_[offsetInOffsets + 1];
return {collection_.begin() + startIndex, collection_.begin() + endIndex};
}

/// get range of objects matching a specified identifier with a specified comparator.
/// <b>WARNING</b>: the comparator has to be written
/// in such a way that the std::equal_range
/// function returns a meaningful range.
/// Not properly written comparators may return
/// an unpredictable range. It is recommended
/// to use only comparators provided with CMSSW release.
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.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw edm::Exception(edm::errors::LogicError, "calling get with comparitor before sorting.");
throw edm::Exception(edm::errors::LogicError, "calling get with comparator before sorting.");

?

}
auto r = std::equal_range(ids_.begin(), ids_.end(), id, comparator);
const_iterator begin, end;
if ((r.first) == ids_.end()) {
begin = end = collection_.end();
return std::make_pair(begin, end);
} else {
begin = collection_.begin() + offsets_[r.first - ids_.begin()];
}
if ((r.second) == ids_.end()) {
end = collection_.end();
} else {
end = collection_.begin() + offsets_[r.second - ids_.begin()];
}
return std::make_pair(begin, end);
}
/// get range of objects matching a specified identifier with a specified comparator.
template <typename CMP>
range get(std::pair<ID, CMP> p) const {
return get(p.first, p.second);
}

/// return number of contained object
size_t size() const { return collection_.size(); }
/// first collection iterator
/// hits in collection are not guaranteed to be in ID order
const_iterator begin() const { return collection_.begin(); }
/// last collection iterator
const_iterator end() const { return collection_.end(); }

// ---------- member functions ---------------------------
/// insert an object range with specified identifier
template <typename CI>
void put(ID id, CI begin, CI end) {
auto i = std::lower_bound(ids_.begin(), ids_.end(), id);
if (i == ids_.end()) {
bool isAlreadySorted = (offsets_.size() == ids_.size());
ids_.emplace_back(std::move(id));
offsets_.push_back(collection_.size());
collection_.insert(collection_.end(), begin, end);
if (not isAlreadySorted) {
//need to update the indirection table used to find the ranges
offsets_.insert(offsets_.begin() + ids_.size() - 1, offsets_.size() - ids_.size());
}
return;
} else if (*i == id) {
throw edm::Exception(edm::errors::LogicError, "trying to insert duplicate entry");
}
if (offsets_.size() == ids_.size()) {
//was sorted, but now it will not be so need to reformat offsets_
std::vector<unsigned int> newOffsets;
newOffsets.reserve(ids_.size() * 2 + 2);
for (std::size_t index = 0; index < ids_.size(); ++index) {
newOffsets.push_back(index);
}
for (std::size_t index = 0; index < ids_.size(); ++index) {
newOffsets.push_back(offsets_[index]);
}
offsets_ = std::move(newOffsets);
}
auto offsetInIds = i - ids_.begin();
ids_.insert(i, id);
offsets_.push_back(collection_.size());
collection_.insert(collection_.end(), begin, end);
offsets_.insert(offsets_.begin() + offsetInIds, offsets_.size() - ids_.size());
}

/// perfor post insert action
void post_insert() {
if (ids_.size() == offsets_.size()) {
//already sorted
return;
}

std::vector<HIT> sortedCollection;
sortedCollection.reserve(collection_.size());

std::vector<unsigned int> offsets;
offsets.reserve(ids_.size());

for (auto id : ids_) {
offsets.push_back(sortedCollection.size());
auto range = get(id);
sortedCollection.insert(sortedCollection.end(), range.first, range.second);
}
offsets_ = std::move(offsets);
collection_ = std::move(sortedCollection);
}

/// first identifier iterator
id_iterator id_begin() const { return ids_.begin(); }
/// last identifier iterator
id_iterator id_end() const { return ids_.end(); }
/// number of contained identifiers
size_t id_size() const { return ids_.size(); }
/// indentifier vector
std::vector<ID> ids() const { return ids_; }
/// direct access to an object in the collection
reference operator[](size_type i) { return collection_[i]; }

/// swap member function
void swap(IdToHitRange<ID, HIT>& other);

//Used by ROOT storage
CMS_CLASS_VERSION(3)

private:
typename std::vector<ID>::const_iterator find(ID id) const {
auto i = std::lower_bound(ids_.begin(), ids_.end(), id);
if (i == ids_.end() or *i != id) {
return ids_.end();
}
return i;
}

// ---------- member data --------------------------------
std::vector<ID> ids_;
std::vector<unsigned int> offsets_;
container collection_;
Comment on lines +200 to +202
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some comments how the ID element gets mapped to collection_ via offsets_?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

};

template <typename ID, typename HIT>
inline void IdToHitRange<ID, HIT>::swap(IdToHitRange<ID, HIT>& other) {
ids_.swap(other.ids_);
offsets_.swap(other.offsets_);
collection_.swap(other.collection_);
}

// free swap function
template <typename ID, typename HIT>
inline void swap(IdToHitRange<ID, HIT>& a, IdToHitRange<ID, HIT>& b) {
a.swap(b);
}

} // namespace edm

#endif
2 changes: 1 addition & 1 deletion DataFormats/Common/test/BuildFile.xml
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
<bin file="Ref_t.cpp">
</bin>

<bin file="ThinnedRefSet_t.cpp">
<bin name ="DataFormatsCommonCatch2Tests" file="ThinnedRefSet_t.cpp,IdToHitRange_t.cpp">
<use name="catch2"/>
</bin>

Expand Down
Loading