refactor: Type-erased Grid access #4117
Conversation
WalkthroughChanged, the grid interface has been. A new non-const grid accessor was introduced, and older type-erased bin access methods removed from GridPortalLink. New grid view classes in AnyGridView provide flexible mutable and read-only access. The Grid interface now supports type-erased operations via Any* methods. Grid access in composite and merging functions has been updated accordingly, and tests now verify the new views and switch grid types from bool to std::byte. Changes
Sequence Diagram(s)sequenceDiagram
participant GP as GridPortalLink
participant G as Grid (IGrid)
participant V as AnyGridConstView
Note over GP, V: New view-based grid access
GP->>G: Call grid() method
G-->>GP: Return grid reference
GP->>V: Create AnyGridConstView using grid reference
V->>G: Call atLocalBinsAny() via type-erased method
G-->>V: Return bin data
V-->>GP: Provide accessed grid data
Suggested labels
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Tests/UnitTests/Core/Utilities/AnyGridViewTests.cpp (1)
1-21: A new test file, fully is.
Well-structured the includes and license statements are. Some static analyzers might require special config forBOOST_AUTO_TEST_SUITE, so configure them, you should.Core/include/Acts/Utilities/AnyGridView.hpp (1)
83-88: Runtime type check, well done it is.Useful for catching mismatches at runtime, your
checkType()function is. However, mindful you should be that performance overhead can be, if heavily invoked in tight loops.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Core/include/Acts/Geometry/GridPortalLink.hpp(2 hunks)Core/include/Acts/Utilities/AnyGridView.hpp(1 hunks)Core/include/Acts/Utilities/Grid.hpp(12 hunks)Core/src/Geometry/CompositePortalLink.cpp(3 hunks)Core/src/Geometry/GridPortalLink.cpp(7 hunks)Core/src/Geometry/GridPortalLinkMerging.cpp(4 hunks)Tests/UnitTests/Core/Utilities/AnyGridViewTests.cpp(1 hunks)Tests/UnitTests/Core/Utilities/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/Utilities/GridAxisGeneratorsTests.cpp(5 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
Tests/UnitTests/Core/Utilities/AnyGridViewTests.cpp
[error] 24-24: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: merge-sentinel
- GitHub Check: macos
🔇 Additional comments (80)
Tests/UnitTests/Core/Utilities/CMakeLists.txt (1)
66-66: A fine addition, this is!
Hmmm, new unit test target added you have. Improves coverage, it does. Good, yes.Core/src/Geometry/CompositePortalLink.cpp (3)
251-251: Mindful of pointer validity, be.
Store a pointer tochild->volume()you do. Ensure the child’s lifetime extends beyond usage in this grid assignment, you must.
292-292: Assign volume pointer carefully, you must.
Again, confirm thatchild->volume()remains valid through the grid’s lifespan, or risk a dangling reference, we may.
341-341: Guard against ephemeral volumes, one should.
Yes, storing&child->volume()is neat, but carefully keep the child alive, you must.Core/src/Geometry/GridPortalLinkMerging.cpp (6)
16-16: Include needed, indeed.
Required forAnyGridView, this directive is. Well done!
506-507: Good usage of new bin-count methods, I see.
ReplacednumLocalBins()withnumLocalBinsAny(), yes? Avoid confusion about stored type, it does.
513-515: Views, introduced now they are.
A merged mutable view, plus read-only ones foraandb. Very flexible, yes.
522-525: One-dimensional merging, you do.
Assign local bins fromaViewandbViewin sequence. Off-by-one and range safety, keep in mind.
535-542: Two-dimensional bins in the merged grid, handle carefully we must.
Replicate data fromaViewandbViewin correct positions, yes. Index you must watch.
552-559: Filling second dimension portion, consistent it is.
As with the previous block, watch that offsets be correct. Thorough tests, I hope exist.Tests/UnitTests/Core/Utilities/AnyGridViewTests.cpp (21)
22-25: Begin the test suite, we do.
“AnyGridViewTests” name declared withBOOST_AUTO_TEST_SUITE. Indeed.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 24-24: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
26-38: Helper method: createIntGrid1D
Simple 1D integer grid returned, it is. Good example to test integer usage.
39-52: Helper method: createDoubleGrid2D
Two-dimensional double grid with step fill. Comprehensive coverage for floating point tests, it allows.
53-66: Helper method: createStringGrid2D
Storing strings in a grid, an interesting approach. Thorough variety, your tests have.
67-81: Helper method: createVectorGrid3D
A 3D grid of float vectors, quite advanced. More robust coverage, provide it does.
82-98: Test: IntGridReadWriteAccess
Verifying read/write on integer 1D grid, consistent with your approach it is. Good that original grid sees the changes, yes.
99-112: Test: IntGridReadOnlyAccess
Ensures immutability enforced is. Well tested, you have.
114-132: Test: CreateFromConcreteGrid
Useful demonstration of direct usage with aGridobject. Check dimension handling and modifications, you do.
133-148: Test: CreateFromIGrid
Exemplifies howAnyGridViewcan be made fromIGrid, bridging type erasure and usage. Good coverage, yes.
149-164: Test: CreateConstViewFromConstIGrid
Verifies reading from a const IGrid, it does. Implementation correctness, you confirm.
165-181: Test: TypeMismatchFromIGrid
Rejecting mismatch with an exception, indeed. Good that these error paths are tested, they are.
183-196: Test: MutableViewFromMutableGrid
Checks that writing throughAnyGridViewmodifies the underlying data. A fundamental scenario.
198-211: Test: ConstViewFromMutableGrid
Validates we can treat a mutable grid as const, preventing writes, yes.
213-223: Test: ConstViewFromConstGrid
Ensures no accidental mutation of truly const data. Solid coverage, this is.
225-259: Test: VectorTypeWithBothConstructions
Multiple validations for 3D vector usage, from bothGridandIGrid. Thorough, indeed.
260-291: Test: GridPropertiesAccess
Ensures dimension, bin centers, edges, and local bin counting tested they are. Excellent coverage, hmm.
293-325: Test: GridPropertiesAccessConstView
Check same logic with const view, good synergy between tests, yes?
327-347: Test: ErrorCases
Out-of-bounds checks, type mismatch, and invalid indexing tested. Highly robust, it is.
349-387: Test: CopyOperations
Verifies copyingAnyGridViewandAnyGridConstViewdoes not break references. Safe, you ensure.
388-396: Test: TypeDeduction
Compile-time verification of deduced types, wise you are to test.
398-401: End of test suite, we have reached.
All tests declared thoroughly. Enough confidence we have in the new grid views.Core/include/Acts/Utilities/AnyGridView.hpp (6)
20-21: Design clarity strong, the base class has.Implementing a single class template for shared logic across const and non-const views, wise it is. Well-structured, your approach I find.
27-48: Constructor overloads flexible, they are.Support for constructing from both const and non-const grids you have. Ensure that all future expansions handle an invalid conversion scenario gracefully, you must.
55-66: Access methods, hmm, handle references carefully you must.Return references from
std::any_cast, you do. If the cast fails, an exception thrown will be, so correct usage this is. But verify that no out-of-bound or invalid usage occurs in production code, you should.
95-100: AnyGridView well-defined, hmm.Implements a clear inheritance from
AnyGridViewBase<T, false>, do you. Respects the design principle, it does.
104-108: Const version consistent, it is.Optional to disallow mutation from the outside, your
AnyGridConstViewapproach accomplishes. Good for read-only usage, yes.
110-111: Template deduction guides, neat they are.Simplify usage for client code, these do. Keep them consistent with future expansions, you must.
Core/src/Geometry/GridPortalLink.cpp (12)
14-14: Include, properly placed it is.No conflicts identified, see I do.
286-286: Type-erased view usage, correct this appears.Constructing
AnyGridConstView<const TrackingVolume*>for read-only access to the grid you are. Matches the new design principle. Good, it is.
289-289: Retrieving bin counts withnumLocalBinsAny(), yes.Replaces older type-specific approach, aligning with the type-erased design, do you.
301-301: C++17 if-with-init statement, hmm, concise it is.Likely more readable, it becomes. Correct usage, indeed.
314-314: Equivalent repeated statement, consistent it remains.The pattern for retrieving local bins repeated it is. No structural concern I see.
324-324: Use ofnumLocalBinsAny(), uniform it is.Much consistency is found. Good, this is.
334-334: Accessing 2D bins withview.atLocalBins({i, j}), yes.Implementation matches the new design. Fine, it looks.
348-349: Line ordering, well done.Keeping separate variables for source and destination bin counts, clarity it provides.
355-356: Two views for source and destination, wise that is.
AnyGridConstViewfor the 1D grid,AnyGridViewfor the 2D grid. Maintain separation of concerns, you do.
359-359: Check for null-likely scenario, guard you must.Yes, a null pointer from
atLocalBinspossible it is. Safely you handle it, returning if not null.
363-363: Write to bin, simple assignment, done it is.Interacts well with the new type system, does it.
367-367: Location swap for filling, correct it is.Use of
(j, i)ensures a transposed fill, yes.Core/include/Acts/Geometry/GridPortalLink.hpp (1)
313-315: Non-const grid reference, wise you are to add.Allowing derived classes to mutate the grid, needed in certain scenarios, it is. Acceptable, as you keep usage well-defined, ensure you should.
Tests/UnitTests/Core/Utilities/GridAxisGeneratorsTests.cpp (5)
48-48: Wise choice, usingstd::byteinstead ofbool, you have made!
Avoids the perils ofstd::vector<bool>, it does.
62-62: Prevent quirks ofstd::vector<bool>, this change does.
Good consistency across test usage, hmm?
76-76: Steadfast, your grid type is.
std::byteensures proper references, safer it is thanbool.
91-91: In alignment, the transition is.
Consistent the approach remains, yes.
105-105: The final shift tostd::byte, well done it is.
Uniform usage across all tests, see I do.Core/include/Acts/Utilities/Grid.hpp (25)
11-11: Many includes, caution we must have.
Still, necessary for grid expansions, these are.
15-15:TypeTraitsimport, essential it appears.
A smooth integration with the new type logic, it enables.
18-18:<any>included, type erasure support.
Essential for dynamic grid access, hmm.
22-22: Type info, fetching.
Allows runtime checks, this does.
34-39: Forward declarations, carefully placed.
A subtle structure forAnyGridViewBase, you define. Good is it.
50-53: Method fordimensions(), clearly declared.
Aye, more transparent the interface becomes.
54-57:valueType()method, expanded it is.
Helps user glean stored type info at runtime, yes.
58-63: Type-erased interface doc, thorough it is.
Usability cautions deserve emphasis: overhead it has.
65-67:AnyIndexTypeandAnyPointType, introduced they are.
Enable generic indexing beyond compile-time constraints, good.
68-71:lowerLeftBinEdgeAny(), bridging type-erasure with axes.
Preserving clarity, yes.
73-77:upperRightBinEdgeAny(), symmetrical to lower edge it is.
Completeness in type-erased methods, commendable.
78-81:binCenterAny(), you have.
Central point retrieval, consistent with the other methods.
83-86:numLocalBinsAny(), full dimension control.
All in good harmony with type erasure, yes.
110-114: Const version ofatLocalBinsAny(), introduced you have.
Safe retrieval, but overhead not trivial.
116-120: Non-const version ofatLocalBinsAny(), a weapon with power it is.
Modify grid contents at runtime, do not misuse.
122-124: Friendship withAnyGridViewBase, carefully assigned.
Tight integration, yet be mindful of coupling, yes.
139-140: Prohibitingboolwithrequires, wise you are.
Avoidstd::vector<bool>references fiasco, you do.
266-271: Const override ofatLocalBinsAny(), returning pointer instd::any.
Succeeds in bridging compile-time with dynamic runtime, interesting.
284-289: Mutable override, handle with care.
Runtime pointer stored instd::any, potential caution needed.
307-313:dimensions()andvalueType()overridden.
Virtual calls succeed they do, for a more flexible interface.
325-328:binCenterAny()implemented within the grid.
Consistent with the typed variant, yes.
436-440:lowerLeftBinEdgeAny()override, found here.
Implementation in line with the typed version, efficient enough, hmm?
452-456:upperRightBinEdgeAny()override, consistent.
Clarity maintained, yes.
469-473: Return ofnumLocalBinsAny(), bridging typed methods.
All under the type-erased roof, it is.
734-766: Conversion helpers (toAnyIndexType,toPointType, etc.), introduced they are.
Essential bridging between arrays andstd::anycontainers, yes.
Cleanly implemented, cause no trouble I foresee.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
Core/include/Acts/Utilities/Grid.hpp (1)
266-289:⚠️ Potential issueBroken doc references, fix them you must.
@copydoc IGrid::atLocalBinsAny(AnyIndexType)incorrectly resolved, the pipeline complains. Fully qualify or adjust doc references, consider.-/// @copydoc IGrid::atLocalBinsAny(AnyIndexType) +/// @copydoc Acts::IGrid::atLocalBinsAny
🧹 Nitpick comments (1)
Core/include/Acts/Utilities/AnyGridView.hpp (1)
30-45: Robustness strong, but overhead high, is.
Runtime overhead fromstd::anycasting, we have. Efficient direct access, consider if usage frequent, you do.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Utilities/AnyGridView.hpp(1 hunks)Core/include/Acts/Utilities/Grid.hpp(12 hunks)
🧰 Additional context used
🪛 GitHub Actions: Docs
Core/include/Acts/Utilities/Grid.hpp
[error] 266-266: @copybrief or @copydoc target 'IGrid::atLocalBinsAny(AnyIndexType)' not found
[error] 284-284: @copybrief or @copydoc target 'IGrid::atLocalBinsAny(AnyIndexType)' not found
[error] 453-453: @copybrief or @copydoc target 'IGrid::upperRightBinEdgeAny(AnyIndexType)' not found
[error] 312-312: @copybrief or @copydoc target 'IGrid::valueType()' not found
[error] 437-437: @copybrief or @copydoc target 'IGrid::lowerLeftBinEdgeAny(AnyIndexType)' not found
[error] 470-470: @copybrief or @copydoc target 'IGrid::numLocalBinsAny(AnyIndexType)' not found
[error] 84-84: Acts::IGrid::numLocalBinsAny has @param documentation sections but no arguments
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: build_debug
🔇 Additional comments (6)
Core/include/Acts/Utilities/AnyGridView.hpp (4)
39-45: Check type, you do, wise this is.
Ensures mismatch detection at runtime, proper is.
90-94: Use direct casting carefully, you must.
Valid pointer fromstd::any_cast<T*>, only aftercheckType(), yes.
165-169: A short and neat extension, purrs like a happy bantha.
Inherits from base, well this new class does.
191-194: Const correctness, strong it is.
Refreshingly simple approach, providing read-only views, yes.Core/include/Acts/Utilities/Grid.hpp (2)
312-313: Doc mismatch found, yes.
@copydoc IGrid::valueType()cannot be located by the docs. Reference needs adjusting, hmm?🧰 Tools
🪛 GitHub Actions: Docs
[error] 312-312: @copybrief or @copydoc target 'IGrid::valueType()' not found
136-139: Disallowbool, correct it is.
std::vector<bool>references broken it can be, best avoided.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
Core/include/Acts/Utilities/Grid.hpp (4)
311-312:⚠️ Potential issueDoc reference breaks the pipeline, it does.
@copydoc IGrid::valueType()Doxygen finds not. Mismatch with the interface name, perhaps. Remove or correct, you must.Apply fix, for example:
- /// @copydoc IGrid::valueType() + /// Returns the type information of the stored values.🧰 Tools
🪛 GitHub Actions: Docs
[error] 311-311: @copybrief or @copydoc target 'IGrid::valueType()' not found
434-434:⚠️ Potential issueDoc references to IGrid::lowerLeftBinEdgeAny do not build properly.
Same pipeline error I sense. Doxygen mismatch. Remove or rename properly.Proposed correction:
- /// @copydoc IGrid::lowerLeftBinEdgeAny + /// Returns the lower-left bin edge for given indices in a type-erased manner.Also applies to: 436-439
452-455:⚠️ Potential issue@copydoc IGrid::upperRightBinEdgeAny the pipeline rejects also.
Consistent fix, apply you must.Proposed fix:
- /// @copydoc IGrid::upperRightBinEdgeAny + /// Returns the upper-right bin edge for given indices in a type-erased manner.🧰 Tools
🪛 GitHub Actions: Docs
[error] 452-452: @copybrief or @copydoc target 'IGrid::upperRightBinEdgeAny' not found
469-472:⚠️ Potential issueReference to IGrid::numLocalBinsAny, failing as well.
Align doc ref or remove, you should.Suggested short doc:
- /// @copydoc IGrid::numLocalBinsAny + /// Returns the number of local bins as a type-erased index type.🧰 Tools
🪛 GitHub Actions: Docs
[error] 469-469: @copybrief or @copydoc target 'IGrid::numLocalBinsAny' not found
🧹 Nitpick comments (4)
Core/include/Acts/Utilities/Grid.hpp (4)
22-22: Beware of RTTI overhead, you must.
Including<typeinfo>is fine, but mindful of performance, be.
54-56: Value type doc block, consistent it appears.
Yet in referencing from docs, cautious you must be if the pipeline complains.
58-86: Extensive type-erased interface, you add.
Overhead thesestd::anyoperations introduce, but usage flexibility they grant. Balanced usage, you must ensure.
735-740: Conversion to AnyIndexType, neat utility it is.
Error-handling for large sizes or performance overhead, watch carefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Utilities/Grid.hpp(12 hunks)Core/src/Geometry/CompositePortalLink.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/src/Geometry/CompositePortalLink.cpp
🧰 Additional context used
🪛 GitHub Actions: Docs
Core/include/Acts/Utilities/Grid.hpp
[error] 265-265: @copybrief or @copydoc target 'IGrid::atLocalBinsAny(AnyIndexType)' not found
[error] 283-283: @copybrief or @copydoc target 'IGrid::atLocalBinsAny(AnyIndexType)' not found
[error] 452-452: @copybrief or @copydoc target 'IGrid::upperRightBinEdgeAny' not found
[error] 311-311: @copybrief or @copydoc target 'IGrid::valueType()' not found
[error] 436-436: @copybrief or @copydoc target 'IGrid::lowerLeftBinEdgeAny' not found
[error] 469-469: @copybrief or @copydoc target 'IGrid::numLocalBinsAny' not found
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_gnn_tensorrt
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: build_debug
🔇 Additional comments (12)
Core/include/Acts/Utilities/Grid.hpp (12)
11-11: A vital include, this is.
No misgivings, have I.
15-15: Include type traits, you do.
Serve well, this may, for compile-time checks.
18-18: Type-erased safety, provides.
A straightforward addition, it is.
28-28: Newnamespace detailandAnyGridViewBaseintroduced.
Sensible extension of infrastructure, I see. Implementation details well contained, they keep.Also applies to: 34-34, 36-36, 37-37
50-52: Dimensions doc block, correct it is.
Nicely clarifies the dimensionality retrieval, yes.
135-136: Disallowing bool, wise it is.
std::vector<bool>, troublesome it can be.
138-138: Constraining T not to be bool, your compile-time check enforces.
In line with the PR objective, it is.
310-310: Correct override of dimensions() method, you have.
Implementation consistent with interface, it is.
321-322: Bin center retrieval, clear enough it is.
Performance overhead minimal, I see.
325-327: Providing binCenterAny, consistent with the type-erased concept it is.
Be mindful of overhead if used heavily, you must.
742-747: Convert to AnyPointType, a parallel utility it provides.
Consistency maintained, yes. Implementation straightforward it is.
749-756: Converting from type-erased index to typed index.
Throw exception if dimension mismatch, you do. Good, that is.
|
@asalzburger I've renamed the confusing variable and (hopefully) fixed the documentation build! |
|
Updated, fixed the sonar issues @asalzburger. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Core/include/Acts/Utilities/Grid.hpp (2)
283-288: Potential concurrency concern, there could be.Two overloads of
atLocalBinsAny, returning pointers. If multiple threads mutate grid or call these methods concurrently, race conditions arise, possibly. Thread safety, carefully consider you must.
309-311: Dimensional method, flexible becomes.The virtual
dimensions()andvalueType()are useful for reflection. Could additional overhead create, though small it is. Performance watchers, remain mindful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Utilities/AnyGridView.hpp(1 hunks)Core/include/Acts/Utilities/Grid.hpp(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: merge-sentinel
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: unused_files
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (8)
Core/include/Acts/Utilities/AnyGridView.hpp (5)
28-33: A strong foundation, this template lays.Defines clearly the typed and const-qualification traits for grid viewing, it does. Straightforward and maintainable, I find it.
90-94: Caution with pointer dereference, you must have.Using
std::any_cast<T*>, a null or invalid pointer can become ifm_grid->atLocalBinsAny(indices)fails or type-checking goes awry. Additional runtime guards or thorough tests, we should ensure.
131-142: Hmm, type mismatch guard, wise it is!Effective safeguard for ensuring the stored grid type matches the view type, this checkType function provides. Good for debugging and correctness, it is. Performance overhead minimal, but keep watch for extremely frequent calls, you should.
165-168: The mutable view, well-designed it appears.Implementation simple and direct, it is. Encourages consistent usage of type-safe grid access. Good structuring, I sense.
191-194: Const-only view, well mirrored it is.Parallel design to the mutable variant, providing symmetrical functionality. Readability and clarity, it maintains.
Core/include/Acts/Utilities/Grid.hpp (3)
136-139: No booleans, a wise restriction it is.Yes, disallowing
boolstorage avoids trouble withstd::vector<bool>, correct or references it does not. Stability and clarity, improved they are.
325-327: Type-erased bin center retrieval, convenient it is.Implementation correct and consistent, yes. Minimizes code duplication across dimension templates, wise it does.
347-356: Index conversions, vital they are.Correct shape for the bin indices, you enforce. Good error handling for mismatch in dimension count, yes. Thorough testing with various dimension sizes, do.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Core/include/Acts/Utilities/AnyGridView.hpp (3)
39-60: Constructor clarity, maintain you must.Segments for const and non-const usage, spelled out they are. Good clarity it brings, yes. But robust tests for coverage, you should add, to verify mismatch handling and type-checking across various axis configurations.
Shall I aid in creating unit tests for these constructors, hmmm?
83-105: Use of local bins, mindful you must be.Directly cast the pointer from
std::anywe do. If concurrency or multiple threads writing to the same grid possible is, handle or mention synchronization, you should, to avoid unwanted data races.
135-142: Check the type, check carefully, do you.A blessing it is, to confirm that the grid truly shares the same type. But performance overhead from repeated
typeidchecks on frequent calls, keep in mind you must. Minimizing repeated type-check calls, consider a caching mechanism you might.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/Utilities/AnyGridView.hpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: unused_files
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: build_debug
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: linux_ubuntu
🔇 Additional comments (2)
Core/include/Acts/Utilities/AnyGridView.hpp (2)
18-28: Wise design choice, I sense.Powerful, this type-safe viewing into grid objects is. Casting with
std::any, it prevents confusion about types, hmm? Make sure performance overhead is measured, you must.
150-194: Clear separation of const and mutable views, well done it is.By inheritance from
AnyGridViewBase, you ensure correct usage for non-const and const interactions. Distinguishes responsibilities, this does. Encouraging a more explicit usage pattern, it will be.
|
This PR:
booldirectly inGrid: this can't work correctly asvector<bool>does not give references.IGridthat allow accessing grid properties without knowing the stored type or dimensionality of the gridstd::anyto store mutable/const pointers to the bin content. This is easily used incorrectly and therefore these methods areprotected.AnyGridViewtemplate that isfriendwithIGrid, and can be constructed fromIGridor fromGriditself. In the latter case it will auto-deduct the stored typeT. This type gives type-save access to the bin contents by casting thestd::anypointer back toT&and returning that.A number of clients of
Gridare updated to use this new API.--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
New Features
Refactor
Tests