Skip to content

Commit

Permalink
fix: avoid undefined unsigned integer behavior in pid size checks (#984)
Browse files Browse the repository at this point in the history
### Briefly, what does this PR introduce?
This avoids undefined behavior flagged by ubsan in the statement
`std::size_t n_tracks = -1;` and in the signed/unsigned integer
comparisons in the pid system, which caused at least MergeTracks to
always fail. It also adds output on the problematic sizes, e.g.
```
[DRICH:DRICHMergedTracks] [error] cannot merge input track collections with different sizes 3, 5
[DRICH:DRICHIrtCherenkovParticleID] [error] radiators have differing numbers of TrackSegments 3, 5, 0
```

### What kind of change does this PR introduce?
- [x] Bug fix (issue #__)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
Yes, should now attempt MergeTracks.

---------

Co-authored-by: Christopher Dilks <[email protected]>
  • Loading branch information
wdconinc and c-dilks authored Sep 19, 2023
1 parent f519a8b commit 926b672
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 17 deletions.
24 changes: 14 additions & 10 deletions src/algorithms/pid/IrtCherenkovParticleID.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

#include "IrtCherenkovParticleID.h"

#include <fmt/ranges.h>

// AlgorithmInit
//---------------------------------------------------------------------------
void eicrecon::IrtCherenkovParticleID::AlgorithmInit(
Expand Down Expand Up @@ -117,20 +119,22 @@ std::map<std::string, std::unique_ptr<edm4eic::CherenkovParticleIDCollection>> e
result.insert({rad_name, std::make_unique<edm4eic::CherenkovParticleIDCollection>()});

// check `in_charged_particles`: each radiator should have the same number of TrackSegments
long num_charged_particles = -1;
for(const auto& [rad_name,charged_particle_list] : in_charged_particles) {
if(num_charged_particles<0) {
num_charged_particles = charged_particle_list->size();
m_log->trace("number of reconstructed charged particles: {}", charged_particle_list->size());
}
else if(num_charged_particles != charged_particle_list->size()) {
m_log->error("radiators have differing numbers of TrackSegments");
return result;
}
std::unordered_map<std::size_t, std::size_t> in_charged_particle_size_distribution;
for(const auto& [rad_name, in_charged_particle] : in_charged_particles) {
++in_charged_particle_size_distribution[in_charged_particle->size()];
}
if (in_charged_particle_size_distribution.size() != 1) {
std::vector<size_t> in_charged_particle_sizes;
std::transform(in_charged_particles.begin(), in_charged_particles.end(),
std::back_inserter(in_charged_particle_sizes),
[](const auto& in_charged_particle) { return in_charged_particle.second->size(); });
m_log->error("radiators have differing numbers of TrackSegments {}", fmt::join(in_charged_particle_sizes, ", "));
return result;
}

// loop over charged particles ********************************************
m_log->trace("{:#<70}","### CHARGED PARTICLES ");
std::size_t num_charged_particles = in_charged_particle_size_distribution.begin()->first;
for(long i_charged_particle=0; i_charged_particle<num_charged_particles; i_charged_particle++) {
m_log->trace("{:-<70}", fmt::format("--- charged particle #{} ", i_charged_particle));

Expand Down
20 changes: 13 additions & 7 deletions src/algorithms/pid/MergeTracks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

#include "MergeTracks.h"

#include <fmt/ranges.h>

// AlgorithmInit
//---------------------------------------------------------------------------
void eicrecon::MergeTracks::AlgorithmInit(std::shared_ptr<spdlog::logger>& logger)
Expand Down Expand Up @@ -30,17 +32,21 @@ std::unique_ptr<edm4eic::TrackSegmentCollection> eicrecon::MergeTracks::Algorith
auto out_tracks = std::make_unique<edm4eic::TrackSegmentCollection>();

// check that all input collections have the same size
std::size_t n_tracks = -1;
std::unordered_map<std::size_t, std::size_t> in_track_collection_size_distribution;
for(const auto& in_track_collection : in_track_collections) {
if(n_tracks == -1)
n_tracks = in_track_collection->size();
else if(n_tracks != in_track_collection->size()) {
m_log->error("input track collections do not have the same size; cannot merge");
return out_tracks;
}
++in_track_collection_size_distribution[in_track_collection->size()];
}
if (in_track_collection_size_distribution.size() != 1) {
std::vector<size_t> in_track_collection_sizes;
std::transform(in_track_collections.begin(), in_track_collections.end(),
std::back_inserter(in_track_collection_sizes),
[](const auto& in_track_collection) { return in_track_collection->size(); });
m_log->error("cannot merge input track collections with different sizes {}", fmt::join(in_track_collection_sizes, ", "));
return out_tracks;
}

// loop over track collection elements
std::size_t n_tracks = in_track_collection_size_distribution.begin()->first;
for(std::size_t i_track=0; i_track<n_tracks; i_track++) {

// create a new output track, and a local container to hold its track points
Expand Down

0 comments on commit 926b672

Please sign in to comment.