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
+84 −179
Open

Remove excess selector code #1587

Changes from 1 commit
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
Prev Previous commit
Next Next commit
Loading
Loading status checks…
Remove suggested const flags to see if the CI behaves better
@simonge
simonge committed Aug 20, 2024
commit 20cb0b4f4feed097227e60da5dd94ac26eb10388
16 changes: 8 additions & 8 deletions src/algorithms/meta/SubDivideFunctors.h
View file
Original file line number Diff line number Diff line change
@@ -13,7 +13,7 @@ namespace eicrecon {
template <auto MemberFunctionPtr>
class RangeSplit {
public:
RangeSplit(const std::vector<std::pair<double, double>>& ranges, bool inside = true)
RangeSplit(std::vector<std::pair<double, double>> ranges, bool inside = true)
: m_ranges(ranges), m_inside(ranges.size(), inside) {}

RangeSplit(const std::vector<std::pair<double, double>>& ranges, const std::vector<bool>& inside)
@@ -43,7 +43,7 @@ class RangeSplit {
}

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

};
@@ -54,7 +54,7 @@ class RangeSplit {
class GeometrySplit {
public:

GeometrySplit(const std::vector<std::vector<long int>>& ids, const std::string& readout, const 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>
@@ -87,9 +87,9 @@ class GeometrySplit {
}
}

const std::vector<std::vector<long int>>& m_ids;
const std::vector<std::string>& m_divisions;
const std::string& m_readout;
std::vector<std::vector<long int>> m_ids;
std::vector<std::string> m_divisions;
std::string m_readout;

mutable std::shared_ptr<std::once_flag> is_init = std::make_shared<std::once_flag>();
mutable dd4hep::DDSegmentation::BitFieldCoder* m_id_dec;
@@ -105,7 +105,7 @@ template <auto... MemberFunctionPtrs>
class ValueSplit {
public:

ValueSplit(const std::vector<std::vector<int>>& ids, bool matching = true)
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>
@@ -130,7 +130,7 @@ class ValueSplit {
}

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

};