refactor: reduce complexity of highly nested blocks #3633
Conversation
github-actions
bot
added
Component - Core
|
|
|
||
| return; |
There was a problem hiding this comment.
nitpick: void return at the end of the function
| dynamic_cast<const Acts::TrapezoidBounds*>(&sbounds); | ||
| std::vector<Acts::Vector2> trapVerts = trapBounds->vertices(); | ||
|
|
||
| for (long unsigned int i = 0; i < trapVerts.size(); i++) { |
There was a problem hiding this comment.
long unsigned int is an interesting type 😄
There was a problem hiding this comment.
Wenn man std::size_t bei Wish.com bestellt.
| } | ||
| } | ||
|
|
||
| return; |
There was a problem hiding this comment.
Same comment as @andiwand gave above
| return true; | ||
| } | ||
|
|
||
| for (int j = -m_cfg.localMaxWindowSize; j <= m_cfg.localMaxWindowSize; j++) { |
There was a problem hiding this comment.
Is our test coverage high enough to be able to see if this is equivalent? /cc @goblirsc
| if (m_cfg.localMaxWindowSize == 0) { | ||
| return true; | ||
| } | ||
|
|
||
| for (int j = -m_cfg.localMaxWindowSize; j <= m_cfg.localMaxWindowSize; j++) { | ||
| if (y + j >= m_cfg.houghHistSize_y) { | ||
| continue; | ||
| } | ||
|
|
||
| for (int i = -m_cfg.localMaxWindowSize; i <= m_cfg.localMaxWindowSize; | ||
| i++) { | ||
| if (i == 0 && j == 0) { | ||
| continue; | ||
| } | ||
|
|
||
| if (x + i >= m_cfg.houghHistSize_x) { | ||
| continue; | ||
| } | ||
|
|
||
| if (houghHist.atLocalBins({y + j, x + i}).first < | ||
| houghHist.atLocalBins({y, x}).first) { | ||
| continue; | ||
| } | ||
|
|
||
| if (houghHist.atLocalBins({y + j, x + i}).first > | ||
| houghHist.atLocalBins({y, x}).first) { | ||
| return false; | ||
| } | ||
|
|
||
| if (houghHist.atLocalBins({y + j, x + i}).second.size() < | ||
| houghHist.atLocalBins({y, x}).second.size()) { | ||
| continue; | ||
| } | ||
|
|
||
| if (houghHist.atLocalBins({y + j, x + i}).second.size() > | ||
| houghHist.atLocalBins({y, x}).second.size()) { | ||
| return false; | ||
| } | ||
|
|
||
| if (j <= 0 && i <= 0) { | ||
| return false; // favor bottom-left (low phi, low neg q/pt) | ||
| } |
There was a problem hiding this comment.
This is a case where I am really not sure if the right hand side is actually more readable than the left hand side. The mental load of tracking early returns and continues is quite significant.
| if (!accept) { | ||
| break; | ||
| } | ||
| for (auto y : std::vector<double>{-dy, dy}) { |
There was a problem hiding this comment.
This is so you abort earlier. However, this is not at all performance critical code, so readability might be preferable over an earlier abort.
There was a problem hiding this comment.
since the loops seem independent something like a product iteration like in python might be nice. then the control flow is easier to handle with a single break. otherwise I agree with paul that the additional looping is not in a critical path so the old version might be preferable
| if (axisTypeA == AxisType::Equidistant && | ||
| axisTypeB == AxisType::Equidistant) { | ||
| auto [rangeA, binsA] = eqExtractor(jAxisA); | ||
| auto [rangeB, binsB] = eqExtractor(jAxisB); | ||
| EqBoundEqClosed ebecAG{rangeA, binsA, rangeB, binsB}; | ||
| auto grid = GridJsonConverter::fromJson<EqBoundEqClosed, ValueType>( | ||
| jGrid, ebecAG); | ||
|
|
||
| return generator.createUpdater(std::move(grid), {bValueA, bValueB}, | ||
| transform); | ||
| } | ||
|
|
||
| if (axisTypeA == AxisType::Equidistant) { | ||
| auto [rangeA, binsA] = eqExtractor(jAxisA); | ||
| EqBoundVarClosed ebvcAG{rangeA, binsA, vExtractor(jAxisB)}; | ||
| auto grid = GridJsonConverter::fromJson<EqBoundVarClosed, ValueType>( | ||
| jGrid, ebvcAG); | ||
|
|
||
| return generator.createUpdater(std::move(grid), {bValueA, bValueB}, | ||
| transform); | ||
| } | ||
|
|
||
| if (axisTypeB == AxisType::Equidistant) { | ||
| auto [rangeB, binsB] = eqExtractor(jAxisB); | ||
| VarBoundEqClosed vbecAG{vExtractor(jAxisA), rangeB, binsB}; | ||
| auto grid = GridJsonConverter::fromJson<VarBoundEqClosed, ValueType>( | ||
| jGrid, vbecAG); | ||
|
|
||
| return generator.createUpdater(std::move(grid), {bValueA, bValueB}, | ||
| transform); | ||
| } |
There was a problem hiding this comment.
This can be an else-if construction.
| if (!accept) { | ||
| break; | ||
| } | ||
| for (auto y : std::vector<double>{-dy, dy}) { | ||
| if (!accept) { | ||
| break; | ||
| } | ||
| for (auto z : std::vector<double>{-dz, dz}) { | ||
| Vector3 edge = etrf * Vector3(x, y, z); | ||
| for (auto& check : options.parseRanges) { | ||
| double val = VectorHelpers::cast(edge, check.first); | ||
| if (val < check.second.first || val > check.second.second) { | ||
| if (!accept) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Can't these breaks be put at the end of the inner loop body?
|
|
||
| if (a.direction() == direction) { | ||
| std::size_t nBinsA = locBinsA.at(0); | ||
| std::size_t nBinsB = locBinsB.at(0); | ||
| std::size_t nBinsCommon = locBinsB.at(1); | ||
|
|
||
| for (std::size_t i = 1; i <= nBinsA; ++i) { | ||
| for (std::size_t j = 1; j <= nBinsCommon; ++j) { | ||
| merged.atLocalBins({i, j}) = a.atLocalBins({i, j}); | ||
| } | ||
| } | ||
|
|
||
| for (std::size_t i = 1; i <= nBinsCommon; ++i) { | ||
| for (std::size_t j = 1; j <= nBinsB; ++j) { | ||
| std::size_t tj = j + nBinsA; | ||
| merged.atLocalBins({i, tj}) = b.atLocalBins({i, j}); | ||
| } | ||
| for (std::size_t i = 1; i <= nBinsB; ++i) { | ||
| for (std::size_t j = 1; j <= nBinsCommon; ++j) { | ||
| std::size_t ti = i + nBinsA; | ||
| merged.atLocalBins({ti, j}) = b.atLocalBins({i, j}); | ||
| } | ||
| } | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
This could be an else-if construct, avoiding an early return.
|
Regarding this use of |
No description provided.