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.

Sign up for GitHub

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

Remove excess selector code #1587

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Remove excess selector code #1587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
7b79806
Removed Selector Code
simonge Aug 20, 2024
2f8fae7
Added functionality to the RangeSplit and ValueSplit functors
simonge Aug 20, 2024
0f2ce1d
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 20, 2024
a601ad4
Changes suggested by sonarcloud
simonge Aug 20, 2024
ccce8f5
Overload the constructor rather than messing around with variants
simonge Aug 20, 2024
c499b4e
Re-add default
simonge Aug 20, 2024
95286e9
More sonarcloud advised changes
simonge Aug 20, 2024
20cb0b4
Remove suggested const flags to see if the CI behaves better
simonge Aug 20, 2024
134a54d
Removed Selector Code
simonge Aug 20, 2024
0e1c3a1
Added functionality to the RangeSplit and ValueSplit functors
simonge Aug 20, 2024
fc9c691
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 20, 2024
5aed707
Changes suggested by sonarcloud
simonge Aug 20, 2024
2c188c2
Overload the constructor rather than messing around with variants
simonge Aug 20, 2024
bbff480
Re-add default
simonge Aug 20, 2024
6b3010e
More sonarcloud advised changes
simonge Aug 20, 2024
9786e24
Remove suggested const flags to see if the CI behaves better
simonge Aug 20, 2024
28e73c0
Merge remote-tracking branch 'origin/main' into Remove-Excess-Selecto…
simonge Aug 28, 2024
320406e
Merge branch 'Remove-Excess-Selector-Code' of github.com:eic/EICrecon…
simonge Aug 28, 2024
5f943e0
Fix indentation
simonge Aug 28, 2024
c336c4f
Created BooleanSplit functor and reverted ValueSplit
simonge Aug 28, 2024
b13bbe8
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 28, 2024
d22b853
Convert BooleanSplit to use float type as the charged field in Recons…
simonge Aug 28, 2024
6b1c511
Merge branch 'Remove-Excess-Selector-Code' of github.com:eic/EICrecon…
simonge Aug 28, 2024
5acacfd
specify float array
simonge Aug 28, 2024
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
83 changes: 78 additions & 5 deletions src/algorithms/meta/SubDivideFunctors.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
#pragma once

#include <algorithms/geo.h>
#include <functional>
#include <vector>
#include <numeric>

namespace eicrecon {

Expand All @@ -13,23 +16,38 @@ namespace eicrecon {
template <auto MemberFunctionPtr>
class RangeSplit {
public:
RangeSplit(std::vector<std::pair<double, double>> ranges, bool inside = true)
: m_ranges(ranges), m_inside(ranges.size(), inside) {}

RangeSplit(std::vector<std::pair<double,double>> ranges) : m_ranges(ranges) {};
RangeSplit(const std::vector<std::pair<double, double>>& ranges, const std::vector<bool>& inside)
: m_ranges(ranges) {
if (inside.size() != ranges.size()) {
throw std::invalid_argument("Size of inside must match the size of ranges");
}
m_inside = inside;
}

template <typename T>
std::vector<int> operator()(T& instance) const {
std::vector<int> ids;
//Check if requested value is within the ranges
for(size_t i = 0; i < m_ranges.size(); i++){
if((instance.*MemberFunctionPtr)() > m_ranges[i].first && (instance.*MemberFunctionPtr)() < m_ranges[i].second){
ids.push_back(i);
if(m_inside[i]){
if((instance.*MemberFunctionPtr)() >= m_ranges[i].first && (instance.*MemberFunctionPtr)() <= m_ranges[i].second){
ids.push_back(i);
}
} else {
if((instance.*MemberFunctionPtr)() < m_ranges[i].first || (instance.*MemberFunctionPtr)() > m_ranges[i].second){
ids.push_back(i);
}
}
}
return ids;
}

private:
std::vector<std::pair<double,double>> m_ranges;
std::vector<bool> m_inside;

};

Expand All @@ -39,7 +57,7 @@ class RangeSplit {
class GeometrySplit {
public:

GeometrySplit(std::vector<std::vector<long int>> ids, std::string readout, std::vector<std::string> divisions)
GeometrySplit(std::vector<std::vector<long int>> ids, const std::string& readout, const std::vector<std::string>& divisions)
: m_ids(ids), m_readout(readout), m_divisions(divisions){};

template <typename T>
Expand Down Expand Up @@ -90,7 +108,8 @@ template <auto... MemberFunctionPtrs>
class ValueSplit {
public:

ValueSplit(std::vector<std::vector<int>> ids) : m_ids(ids) {};
ValueSplit( std::vector<std::vector<int>> ids, bool matching = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also const ref the ids arg here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, optionally, I've wondered about a TrueFalseSplit functor a number of times. That's really just a ValueSplit (which is how this gets implemented in plugins), but it could have the benefit of accepting a function that returns a bool, which is semantically cleaner than relying on bool to integer conversions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You would have to move away from the simple std::find implementation for anything other than ==. I'll have another think, as changing the functors already there is making them less clear. It should be possible to pass an array of boolean operators along with values and member functions to a functor.

After the vertexing discussion, access to min/max might be nice too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BooleanSplit (name can still be changed) should be able to perform the jobs of RangeSplit and ValueSplit where an array of functions which return a bool from two inputs is passed alongside the array of values to compare. The bools are all reduced by and rather than trying to mix in some more complexity.

What is missing the the ability to compare types other than float, I'm not sure how to approach for instance asking for e.g. ReconstructedParticles with PDG "equal_to" and a goodnessOfPID "greater_than". Maybe it's not important, could be done in a few steps, or this sort of thing is not what we'll want to do in eicrecon.

: m_ids(ids), m_matching(matching) {};

template <typename T>
std::vector<int> operator()(T& instance) const {
Expand All @@ -107,6 +126,60 @@ class ValueSplit {

private:
std::vector<std::vector<int>> m_ids;
bool m_matching = true;

};

// ----------------------------------------------------------------------------
// Functor to split collection based on any number of collection values
// ----------------------------------------------------------------------------
template <auto... MemberFunctionPtrs>
class BooleanSplit {
public:
using ComparisonFunction = std::function<bool(float, float)>;

BooleanSplit( std::vector<std::vector<float>> ids, ComparisonFunction comparison)
: m_ids(ids), m_comparisons(ids.size(), comparison) {};

BooleanSplit( std::vector<float> ids, ComparisonFunction comparison)
: m_ids(1,ids), m_comparisons(ids.size(), comparison) {};


BooleanSplit( std::vector<std::vector<float>> ids, std::vector<ComparisonFunction> comparisons)
: m_ids(ids) {
if (ids.size() != comparisons.size()) {
throw std::invalid_argument("Size of values to compare must match the size of boolean functions");
}
m_comparisons = comparisons;
}

template <typename T>
std::vector<int> operator()(T& instance) const {
std::vector<int> ids;
// Check if requested value matches any configuration combinations
std::vector<float> values;
(values.push_back((instance.*MemberFunctionPtrs)()), ...);
for (size_t i = 0; i < m_ids.size(); ++i){
if(compareVectors(m_ids[i], values, m_comparisons)){
ids.push_back(i);
}
}
return ids;
}

private:
std::vector<std::vector<float>> m_ids;
std::vector<ComparisonFunction> m_comparisons;

static bool compareVectors(const std::vector<float>& vec1, const std::vector<float>& vec2, const std::vector<ComparisonFunction>& comparisons) {
for (size_t i = 0; i < vec1.size(); ++i) {
if (!comparisons[i](vec1[i], vec2[i])) {
return false;
}
}
return true;
}


};

Expand Down
45 changes: 0 additions & 45 deletions src/algorithms/reco/ChargedMCParticleSelector.h
View file
Open in desktop

This file was deleted.

45 changes: 0 additions & 45 deletions src/algorithms/reco/ChargedReconstructedParticleSelector.h
View file
Open in desktop

This file was deleted.

41 changes: 0 additions & 41 deletions src/global/reco/ChargedMCParticleSelector_factory.h
View file
Open in desktop

This file was deleted.

41 changes: 0 additions & 41 deletions src/global/reco/ChargedReconstructedParticleSelector_factory.h
View file
Open in desktop

This file was deleted.

8 changes: 6 additions & 2 deletions src/global/reco/reco.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <edm4hep/MCParticle.h>
#include <map>
#include <memory>
#include <functional>

#include "algorithms/interfaces/WithPodConfig.h"

Expand All @@ -27,6 +28,8 @@
#include "extensions/jana/JOmniFactoryGeneratorT.h"
#include "factories/meta/CollectionCollector_factory.h"
#include "factories/meta/FilterMatching_factory.h"
#include "factories/meta/SubDivideCollection_factory.h"
#include "algorithms/meta/SubDivideFunctors.h"
#include "factories/reco/FarForwardNeutronReconstruction_factory.h"
#ifdef USE_ONNX
#include "factories/reco/InclusiveKinematicsML_factory.h"
Expand All @@ -41,13 +44,13 @@
#include "factories/reco/HadronicFinalState_factory.h"
#endif
#include "factories/reco/UndoAfterBurnerMCParticles_factory.h"
#include "global/reco/ChargedReconstructedParticleSelector_factory.h"
#include "global/reco/MC2SmearedParticle_factory.h"
#include "global/reco/MatchClusters_factory.h"
#include "global/reco/ReconstructedElectrons_factory.h"
#include "global/reco/ScatteredElectronsEMinusPz_factory.h"
#include "global/reco/ScatteredElectronsTruth_factory.h"


extern "C" {
void InitPlugin(JApplication *app) {
InitJANAPlugin(app);
Expand Down Expand Up @@ -248,10 +251,11 @@ void InitPlugin(JApplication *app) {
app
));

app->Add(new JOmniFactoryGeneratorT<ChargedReconstructedParticleSelector_factory>(
app->Add(new JOmniFactoryGeneratorT<SubDivideCollection_factory<edm4eic::ReconstructedParticle>>(
"GeneratedChargedParticles",
{"GeneratedParticles"},
{"GeneratedChargedParticles"},
{.function = BooleanSplit<&edm4eic::ReconstructedParticle::getCharge>{std::vector<float>{0.0},std::not_equal_to<float>{}}},
app
));

Expand Down
Loading