Tiles #922
Tiles #922
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several changes related to the management and testing of tile and particle functionalities within the PHARE project. Key additions include new classes for handling particle tiles and tile sets, as well as corresponding unit tests for these components. The CMake configuration is updated to include new test directories and executables, ensuring that the new functionalities are properly integrated into the build process. Overall, the changes enhance the testing framework and expand the core data management capabilities. Changes
Possibly related PRs
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (11)
tests/core/data/particles/test_particle_tiles.cpp (1)
10-16: Consider adding dimension validation.While the type aliases are well-structured, consider adding static assertions to validate dimension values at compile-time. This would prevent instantiation with invalid dimensions.
template<std::size_t dimension> using ParticleTile_t = ParticleTile<ParticleArray<dimension>>; +static_assert(dimension >= 1 && dimension <= 3, "Dimension must be between 1 and 3");src/core/data/particles/particle_tile.hpp (1)
22-48: Consider additional improvements for ParticleTileSet.
- Add class-level documentation explaining its purpose and relationship with ParticleTile
- Consider marking select_particles as noexcept(false) to explicitly indicate it may throw
- Consider returning ParticleArray&& to enable move semantics
Example documentation:
/** * @brief Manages a set of particle tiles and provides operations for particle selection * @tparam ParticleArray The type of particle array stored in the tiles */tests/core/data/tiles/test_tile.cpp (5)
1-3: Remove unused header includes.The headers
<ctype.h>and<string>are included but not used in the code.-#include <ctype.h> -#include <string>
85-88: Fix typo in test name.The test name "cluserSetSizeIsCorrect" contains a typo. It should be "clusterSetSizeIsCorrect".
132-134: Consider extracting magic numbers into named constants.The expected number calculation uses magic numbers. Consider extracting these into named constants for better readability and maintainability.
- auto expected_nbr = std::pow(7, this->dimension); + static constexpr int TILES_PER_DIMENSION = 7; + auto expected_nbr = std::pow(TILES_PER_DIMENSION, this->dimension);
109-124: Consider optimizing overlap check performance.The nested loops create O(n²) complexity. Consider using a spatial partitioning structure or optimizing the algorithm for better performance with large tile sets.
133-133: Fix spelling in method name.The method name "export_overlaped_with" contains spelling errors. It should be "export_overlapped_with".
src/core/data/tiles/tile_set.hpp (4)
70-74: Add a const overload for the 'at' method to allow const accessThe
atmethod currently provides only a non-const version. Adding a const overload will allow users to accesscells_in a read-only context.Apply this diff to add the const overload:
template<typename... Index> NO_DISCARD auto& at(Index... indexes) { return cells_(indexes...); } + template<typename... Index> + NO_DISCARD auto const& at(Index... indexes) const + { + return cells_(indexes...); + }
77-87: Improve exception message in 'consistent_tile_size_' for clarityThe exception message can be enhanced to include actual tile and box sizes for better debugging.
Apply this diff to improve the exception message:
if (box_.shape()[idim] < tile_size_[idim]) { - throw std::runtime_error("tile size larger than box size in dimension " - + std::to_string(idim)); + throw std::runtime_error("Tile size " + std::to_string(tile_size_[idim]) + + " is larger than box size " + std::to_string(box_.shape()[idim]) + + " in dimension " + std::to_string(idim)); }
101-136: Refactor 'make_tiles_' to reduce code duplication and improve maintainabilityThe
make_tiles_function contains repetitive code blocks for different dimensions. Consider generalizing the tile creation process to handle any dimension, reducing code duplication and enhancing maintainability.For example, you can use recursive functions or loop unrolling to generalize over dimensions:
void make_tiles_() { auto const compute_indices = [&](auto... idx) { // Calculate the linear index based on multi-dimensional indices std::size_t linear_idx = /* computation based on idx... */; tiles_[linear_idx].lower = /* compute lower bounds */; tiles_[linear_idx].upper = /* compute upper bounds */; }; // Recursively iterate over dimensions // Pseudo-code as implementation would be complex for this format recursive_dimension_loop(compute_indices); }This approach eliminates the need for
if constexprand nested loops specific to each dimension.
25-33: Rename variable 's' to 'shape' for better readabilityUsing more descriptive variable names improves code readability and maintainability.
Apply this diff to rename the variable:
std::array<int, dimension> s; auto bs = box.shape(); for (auto i = 0u; i < dimension; ++i) { auto const div = (bs[i] + tile_size_[i] - 1) / tile_size_[i]; - s[i] = div; + shape[i] = div; } - return s; + return shape;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
res/cmake/test.cmake(1 hunks)src/core/data/particles/particle_tile.hpp(1 hunks)src/core/data/tiles/tile_set.hpp(1 hunks)tests/core/data/particles/CMakeLists.txt(1 hunks)tests/core/data/particles/test_particle_tiles.cpp(1 hunks)tests/core/data/tiles/CMakeLists.txt(1 hunks)tests/core/data/tiles/test_tile.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/core/data/tiles/CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (2)
src/core/data/particles/particle_tile.hpp (1)
Pattern **/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/tiles/tile_set.hpp (1)
Pattern **/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (8)
tests/core/data/particles/CMakeLists.txt (1)
23-23: LGTM! Test configuration follows project conventions.
The new test executable is properly configured using the existing _particles_test function, maintaining consistency with other particle tests in the project.
tests/core/data/particles/test_particle_tiles.cpp (2)
1-8: LGTM! Headers and namespace setup looks good.
The includes are well-organized, following the convention of external dependencies first (Google Test) followed by internal headers. The namespace usage is clear and appropriate.
35-40: LGTM! Main function follows Google Test best practices.
The main function correctly initializes the Google Test framework and runs the tests.
src/core/data/particles/particle_tile.hpp (1)
1-6: LGTM! Header setup follows best practices.
The header guards and includes are properly structured and minimal.
res/cmake/test.cmake (1)
32-32: LGTM! The test directory addition follows the established pattern.
The new test directory is appropriately placed within the core data test group and the change aligns with the PR objectives.
Let's verify the existence of the required test files:
✅ Verification successful
Test directory structure and files are properly set up
The verification confirms that all essential components are in place:
- Test directory exists at
tests/core/data/tiles/ - Test file
test_tile.cppis present CMakeLists.txtis available for build configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of test files in the new test directory
# Test: Check if the directory and essential files exist
fd -t d "tiles$" tests/core/data
fd -t f "^test.*\.(cpp|hpp)$" tests/core/data/tiles
fd -t f "CMakeLists.txt$" tests/core/data/tiles
Length of output: 234
tests/core/data/tiles/test_tile.cpp (3)
17-31: LGTM! Well-designed base test class.
The TileTestBase class provides a clean and reusable foundation for tile tests with proper member initialization.
46-57: Verify box dimensions in TileTestBoxShapeMultipleTileSize.
The class name suggests testing scenarios where box shape is a multiple of tile size (4), but the box dimensions (47) don't appear to be multiples of the tile size. This seems inconsistent with the class's intended purpose.
Consider adjusting the box dimensions to be multiples of the tile size (e.g., 48 instead of 47) to match the class name's intention.
177-182: LGTM! Standard Google Test main function.
The main function correctly initializes the Google Test framework and runs the tests.
| template<typename ParticleTile> | ||
| class ParticleTileTest : public ::testing::Test | ||
| { | ||
| protected: | ||
| ParticleTileTest() {} | ||
| }; | ||
|
|
||
| TYPED_TEST_SUITE(ParticleTileTest, DimParticleTiles); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test fixture with common test scenarios.
The current fixture is minimal. Consider adding:
- Common test data setup
- Helper methods for validation
- Test cases for error conditions
Example enhancement:
template<typename ParticleTile>
class ParticleTileTest : public ::testing::Test {
protected:
static constexpr auto dim = ParticleTile::dimension;
Box<int, dim> defaultBox{ConstArray<int, dim>(0), ConstArray<int, dim>(50)};
ConstArray<std::size_t, dim> defaultTileSize{4};
void SetUp() override {
// Setup common test state
}
void TearDown() override {
// Cleanup if needed
}
// Helper methods for common validations
void validateTileProperties(const ParticleTile& tile) {
// Add validation logic
}
};| TYPED_TEST(ParticleTileTest, constructs) | ||
| { | ||
| constexpr auto dim = TypeParam::dimension; | ||
| Box<int, dim> box{ConstArray<int, dim>(0), ConstArray<int, dim>(50)}; | ||
| auto const tile_size = PHARE::core::ConstArray<std::size_t, dim>(4); | ||
| TileSet<TypeParam> particleTileSet{box, tile_size}; | ||
| } |
There was a problem hiding this comment.
Add comprehensive test coverage.
The current test only verifies basic construction. Consider adding tests for:
- Tile boundary conditions
- Particle insertion and retrieval
- Invalid box dimensions
- Edge cases for tile sizes
- Error conditions
Example additional test:
TYPED_TEST(ParticleTileTest, handlesInvalidTileSize)
{
constexpr auto dim = TypeParam::dimension;
Box<int, dim> box{ConstArray<int, dim>(0), ConstArray<int, dim>(50)};
auto const invalid_tile_size = PHARE::core::ConstArray<std::size_t, dim>(0);
EXPECT_THROW({
TileSet<TypeParam> particleTileSet{box, invalid_tile_size};
}, std::invalid_argument);
}
TYPED_TEST(ParticleTileTest, correctlyPartitionsTiles)
{
constexpr auto dim = TypeParam::dimension;
Box<int, dim> box{ConstArray<int, dim>(0), ConstArray<int, dim>(10)};
auto const tile_size = PHARE::core::ConstArray<std::size_t, dim>(2);
TileSet<TypeParam> particleTileSet{box, tile_size};
// Verify number of tiles and their positions
// Add assertions here
}| template<typename ParticleArray> | ||
| struct ParticleTile : public Box<int, ParticleArray::dimension> | ||
| { | ||
| // *this: box constructed first | ||
| ParticleArray domainParticles_{*this}; | ||
| ParticleArray patchGhostParticles_{*this}; | ||
| ParticleArray levelGhostParticles_{*this}; | ||
| ParticleArray levelGhostParticlesNew_{*this}; | ||
| ParticleArray levelGhostParticlesOld_{*this}; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and consider class semantics.
Several improvements are recommended:
- Add documentation explaining the purpose and lifecycle of each particle array type
- Consider making this a class with explicit access control
- Define move/copy semantics explicitly or delete them
- Add a virtual destructor since this is a base class
Here's a suggested improvement:
template<typename ParticleArray>
-struct ParticleTile : public Box<int, ParticleArray::dimension>
+class ParticleTile : public Box<int, ParticleArray::dimension>
{
+public:
+ // Virtual destructor for proper cleanup in inheritance hierarchy
+ virtual ~ParticleTile() = default;
+
+ // Default move operations
+ ParticleTile(ParticleTile&&) = default;
+ ParticleTile& operator=(ParticleTile&&) = default;
+
+ // Prevent copying as it might be expensive
+ ParticleTile(const ParticleTile&) = delete;
+ ParticleTile& operator=(const ParticleTile&) = delete;
+
+ // Domain particles represent...
ParticleArray domainParticles_{*this};
+ // Patch ghost particles are used for...
ParticleArray patchGhostParticles_{*this};
+ // Level ghost particles represent...
ParticleArray levelGhostParticles_{*this};
+ // New level ghost particles are used during...
ParticleArray levelGhostParticlesNew_{*this};
+ // Old level ghost particles maintain...
ParticleArray levelGhostParticlesOld_{*this};
};| auto select_particles(Box<int, dimension> const& from) const | ||
| { | ||
| ParticleArray selected; | ||
|
|
||
| auto overlaped_tiles = export_overlaped_with(from); | ||
| for (auto const& [complete, tile] : overlaped_tiles) | ||
| { | ||
| if (complete) | ||
| { | ||
| selected.push_back(tile.domainParticles_); | ||
| } | ||
| else | ||
| { | ||
| auto intersection = tile * from; | ||
| tile.domainParticles.export_to(*intersection, selected); | ||
| } | ||
| } | ||
| return selected; | ||
| } |
There was a problem hiding this comment.
Fix critical issues in select_particles method.
Several issues need to be addressed:
- Line 42:
domainParticlesshould bedomainParticles_ - Line 32: Method name has a typo:
overlapedshould beoverlapped - Performance: Use
const bool&for the complete flag in the loop - Add error handling for invalid box dimensions
Here's the corrected implementation:
auto select_particles(Box<int, dimension> const& from) const
{
ParticleArray selected;
- auto overlaped_tiles = export_overlaped_with(from);
- for (auto const& [complete, tile] : overlaped_tiles)
+ if (from.dimension() != dimension)
+ throw std::invalid_argument("Box dimension mismatch");
+
+ auto overlapped_tiles = export_overlapped_with(from);
+ for (auto const& [complete, tile] : overlapped_tiles)
{
if (complete)
{
selected.push_back(tile.domainParticles_);
}
else
{
auto intersection = tile * from;
- tile.domainParticles.export_to(*intersection, selected);
+ tile.domainParticles_.export_to(*intersection, selected);
}
}
return selected;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto select_particles(Box<int, dimension> const& from) const | |
| { | |
| ParticleArray selected; | |
| auto overlaped_tiles = export_overlaped_with(from); | |
| for (auto const& [complete, tile] : overlaped_tiles) | |
| { | |
| if (complete) | |
| { | |
| selected.push_back(tile.domainParticles_); | |
| } | |
| else | |
| { | |
| auto intersection = tile * from; | |
| tile.domainParticles.export_to(*intersection, selected); | |
| } | |
| } | |
| return selected; | |
| } | |
| auto select_particles(Box<int, dimension> const& from) const | |
| { | |
| ParticleArray selected; | |
| if (from.dimension() != dimension) | |
| throw std::invalid_argument("Box dimension mismatch"); | |
| auto overlapped_tiles = export_overlapped_with(from); | |
| for (auto const& [complete, tile] : overlapped_tiles) | |
| { | |
| if (complete) | |
| { | |
| selected.push_back(tile.domainParticles_); | |
| } | |
| else | |
| { | |
| auto intersection = tile * from; | |
| tile.domainParticles_.export_to(*intersection, selected); | |
| } | |
| } | |
| return selected; | |
| } |
| NO_DISCARD auto export_overlaped_with(Box<int, dimension> const& box) const | ||
| { | ||
| std::vector<std::pair<bool, Tile>> overlaped; | ||
| for (auto const& tile : tiles_) | ||
| { | ||
| auto overlap = box * Box<int, dimension>{tile.lower, tile.upper}; | ||
| if (overlap) | ||
| { | ||
| auto complete_overlap = (*overlap).size() == tile.size(); | ||
| overlaped.emplace_back(complete_overlap, tile); | ||
| } | ||
| } | ||
| return overlaped; | ||
| } |
There was a problem hiding this comment.
Fix typo in method and variable names: 'overlaped' should be 'overlapped'
There is a typo in the method name export_overlaped_with and the variable overlaped. The correct spelling is 'overlapped'.
Apply this diff to correct the typos:
- NO_DISCARD auto export_overlaped_with(Box<int, dimension> const& box) const
+ NO_DISCARD auto export_overlapped_with(Box<int, dimension> const& box) const
- std::vector<std::pair<bool, Tile>> overlaped;
+ std::vector<std::pair<bool, Tile>> overlapped;Committable suggestion skipped: line range outside the PR's diff.
🛠️ Refactor suggestion
Avoid unnecessary copying in 'export_overlapped_with' by returning pointers to tiles
Currently, export_overlapped_with returns a vector of pairs containing copies of Tile. To improve performance and avoid unnecessary copying, consider returning pointers to the tiles instead.
Apply this diff to modify the vector to store pointers to Tile objects:
- std::vector<std::pair<bool, Tile>> overlapped;
+ std::vector<std::pair<bool, Tile const*>> overlapped;
for (auto const& tile : tiles_)
{
auto overlap = box * Box<int, dimension>{tile.lower, tile.upper};
if (overlap)
{
auto complete_overlap = (*overlap).size() == tile.size();
- overlapped.emplace_back(complete_overlap, tile);
+ overlapped.emplace_back(complete_overlap, &tile);
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/core/data/tiles/tile_set.hpp (2)
122-170: Refactormake_tiles_()function to reduce code duplicationThe
make_tiles_()function contains nested loops with repeated code blocks for handling different dimensions (1D, 2D, 3D) usingif constexpr. This results in code duplication and can make the code harder to maintain or extend for higher dimensions. Consider refactoring this function to eliminate duplication by using a more generic approach.For example, you could iterate over all possible indices in a single loop and compute the lower and upper bounds for each dimension generically:
void make_tiles_() { for (size_t idx = 0; idx < tiles_.size(); ++idx) { auto indices = compute_indices(idx, shape_); for (size_t dim = 0; dim < dimension; ++dim) { tiles_[idx].lower[dim] = box_.lower[dim] + indices[dim] * tile_size_[dim]; tiles_[idx].upper[dim] = tiles_[idx].lower[dim] + size_me(dim, indices[dim]) - 1; } } }This approach reduces duplication and improves maintainability, making it easier to adjust for different dimensions in the future.
103-107: Markmake_view()as aconstmethodThe
make_view()method does not modify any member variables and can be marked asconst. This allows it to be called on constant instances ofTileSetand clearly communicates that the method does not alter the object's state.Apply this diff to mark the method as
const:auto make_view() const { return TileSetView<Tile>{box_, tile_size_, shape_, tiles_.data(), tiles_.size(), cells_.data(), cells_.size()}; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/core/data/tiles/tile_set.hpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/core/data/tiles/tile_set.hpp (1)
Pattern **/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
| for (auto idim = 0u; idim < dimension; ++idim) | ||
| { | ||
| if (box_.shape()[idim] < tile_size_[idim]) | ||
| { | ||
| throw std::runtime_error("tile size larger than box size in dimension " | ||
| + std::to_string(idim)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add check to prevent zero tile_size_ elements to avoid division by zero
Currently, the consistent_tile_size_() function checks if each tile_size_[idim] is larger than the corresponding box_.shape()[idim]. However, it does not check if tile_size_[idim] is zero. If any tile_size_[idim] is zero, it will result in a division by zero error in subsequent calculations, such as in the constructor when computing div.
Apply this diff to add a check for zero tile_size_ elements:
void consistent_tile_size_() const
{
for (auto idim = 0u; idim < dimension; ++idim)
{
+ if (tile_size_[idim] == 0)
+ {
+ throw std::runtime_error("tile size cannot be zero in dimension " + std::to_string(idim));
+ }
if (box_.shape()[idim] < tile_size_[idim])
{
throw std::runtime_error("tile size larger than box size in dimension "
+ std::to_string(idim));
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (auto idim = 0u; idim < dimension; ++idim) | |
| { | |
| if (box_.shape()[idim] < tile_size_[idim]) | |
| { | |
| throw std::runtime_error("tile size larger than box size in dimension " | |
| + std::to_string(idim)); | |
| } | |
| } | |
| for (auto idim = 0u; idim < dimension; ++idim) | |
| { | |
| if (tile_size_[idim] == 0) | |
| { | |
| throw std::runtime_error("tile size cannot be zero in dimension " + std::to_string(idim)); | |
| } | |
| if (box_.shape()[idim] < tile_size_[idim]) | |
| { | |
| throw std::runtime_error("tile size larger than box size in dimension " | |
| + std::to_string(idim)); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
tests/core/data/tiles/test_tile.cpp (3)
34-44: Consider more descriptive test class names.The current class names
TileTestBoxShapeNotMultipleTileSizeandTileTestBoxShapeMultipleTileSizeare quite long and could be more descriptive. Consider names that better reflect the test scenarios:
TileTestWithUnalignedBoxTileTestWithAlignedBoxAlso applies to: 47-57
78-83: Define constants for magic numbers.The test uses magic number
14for shape comparison. Consider defining a constant with a meaningful name to improve readability and maintainability.+ static constexpr size_t EXPECTED_TILES_PER_DIM = 14; auto const& shape = this->tileSet.shape(); for (auto i = 0u; i < this->dimension; ++i) - EXPECT_EQ(shape[i], 14); + EXPECT_EQ(shape[i], EXPECTED_TILES_PER_DIM);
1-189: Consider adding tests for edge cases and error scenarios.While the test coverage is good for basic functionality, consider adding tests for:
- Zero-sized boxes
- Invalid inputs (negative sizes, etc.)
- Performance benchmarks for large tile sets
Would you like me to help generate these additional test cases?
src/core/data/tiles/tile_set.hpp (2)
106-110: Document view lifetime requirements and add validation.The
make_viewmethod creates a view into the TileSet's data but lacks documentation about lifetime requirements.Consider:
- Adding method documentation
- Adding validation for empty containers
- Using
[[nodiscard]]attribute+ /** + * Creates a non-owning view of the tile set's data. + * The returned view is valid only as long as this TileSet exists. + * @return TileSetView pointing to this TileSet's data + */ + [[nodiscard]] auto make_view() { + if (tiles_.empty() || cells_.empty()) + { + throw std::runtime_error("Cannot create view of empty TileSet"); + } return TileSetView<Tile>{box_, tile_size_, shape_, tiles_.data(), tiles_.size(), cells_.data(), cells_.shape()}; }
177-186: Add validation and documentation for cell tagging.The
tag_cells_method lacks validation and documentation about its purpose and requirements.Consider:
- Adding method documentation
- Adding validation for cell access
- Using debug assertions
+ /** + * Associates each cell with its containing tile by storing tile pointers. + * Assumes that cells_ has been properly sized to match the box dimensions. + */ void tag_cells_() { + assert(!tiles_.empty() && "Cannot tag cells with empty tiles"); for (auto& tile : tiles_) { for (auto const& cell : tile) { + assert(cells_.contains(cell) && "Cell index out of bounds"); cells_(cell) = &tile; } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/core/data/tiles/tile_set.hpp(1 hunks)tests/core/data/tiles/test_tile.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/core/data/tiles/tile_set.hpp (1)
Pattern **/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (2)
tests/core/data/tiles/test_tile.cpp (2)
1-16: LGTM! Headers are well-organized.
Headers are properly organized with system headers first, followed by project headers, and then test framework headers.
184-189: LGTM! Standard Google Test main function.
The main function correctly initializes the Google Test framework and runs all tests.
tests/core/data/tiles/test_tile.cpp
Outdated
| TYPED_TEST(TileTestBoxShapeNotMultipleTileSize, getView) | ||
| { | ||
| auto view = this->tileSet.make_view(); | ||
| } |
There was a problem hiding this comment.
Empty test case needs implementation.
The getView test case is empty and doesn't verify any behavior. Either implement meaningful assertions or remove the test case.
Consider implementing assertions to verify the view's properties:
TYPED_TEST(TileTestBoxShapeNotMultipleTileSize, getView)
{
auto view = this->tileSet.make_view();
EXPECT_EQ(view.dimension(), this->dimension);
EXPECT_EQ(view.size(), this->tileSet.size());
// Add more assertions to verify view's behavior
}| TileSet(Box<int, dimension> const& box, std::array<std::size_t, dimension> const& tile_size) | ||
| : box_{box} | ||
| , tile_size_{tile_size} | ||
| , shape_{[&]() { | ||
| std::array<std::uint32_t, dimension> s; | ||
| auto bs = box.shape(); | ||
| for (auto i = 0u; i < dimension; ++i) | ||
| { | ||
| auto const div = (bs[i] + tile_size_[i] - 1) / tile_size_[i]; | ||
| s[i] = div; | ||
| } | ||
| return s; | ||
| }()} | ||
| , tiles_(product(shape_)) | ||
| , cells_{box.shape().template toArray<std::uint32_t>()} | ||
| { | ||
| consistent_tile_size_(); | ||
| make_tiles_(); | ||
| tag_cells_(); | ||
| } |
There was a problem hiding this comment.
Add validation for zero tile sizes and document shape calculation.
The shape calculation uses a ceiling division pattern but lacks validation for zero tile sizes.
Consider:
- Adding validation for zero tile sizes
- Documenting the shape calculation formula
- Using a named function for ceiling division
TileSet(Box<int, dimension> const& box, std::array<std::size_t, dimension> const& tile_size)
: box_{box}
, tile_size_{tile_size}
, shape_{[&]() {
std::array<std::uint32_t, dimension> s;
auto bs = box.shape();
for (auto i = 0u; i < dimension; ++i)
{
+ if (tile_size_[i] == 0)
+ {
+ throw std::runtime_error("tile size cannot be zero in dimension " + std::to_string(i));
+ }
auto const div = (bs[i] + tile_size_[i] - 1) / tile_size_[i]; // ceiling division
s[i] = div;
}
return s;
}()}Committable suggestion skipped: line range outside the PR's diff.
src/core/data/tiles/tile_set.hpp
Outdated
| template<typename Tile> | ||
| class TileSetView | ||
| { | ||
| public: | ||
| static auto constexpr dimension = Tile::dimension; | ||
|
|
||
| TileSetView(Box<int, dimension> const& box, std::array<std::size_t, dimension> const& tile_size, | ||
| std::array<std::uint32_t, dimension> const& shape, Tile* tiles, | ||
| std::uint32_t tile_nbr, Tile** cells, | ||
| std::array<std::uint32_t, dimension> const& nbr_cells) | ||
| : box_{box} | ||
| , tile_size_{tile_size} | ||
| , shape_{shape} | ||
| , tiles_{tiles, tile_nbr} | ||
| , cells_{cells, nbr_cells} | ||
| { | ||
| } | ||
|
|
||
| private: | ||
| Box<int, dimension> const box_; | ||
| std::array<std::size_t, dimension> tile_size_; | ||
| std::array<std::uint32_t, dimension> shape_; | ||
| Span<Tile> tiles_; | ||
| NdArrayView<dimension, Tile*> cells_; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document ownership semantics and add noexcept specifications.
The TileSetView class uses raw pointers without clear documentation about ownership semantics. While the use of Span and NdArrayView suggests non-owning views, this should be explicitly documented.
Consider:
- Adding class-level documentation explaining the view semantics
- Marking the constructor as
noexcept - Adding
[[nodiscard]]to getters if you plan to add them - Using
gsl::spanorstd::span(C++20) instead of custom Span if possible
+/**
+ * A non-owning view into a TileSet's data.
+ * This class provides a lightweight view of tiles and cells without taking ownership.
+ * The viewed data must outlive this view.
+ */
template<typename Tile>
class TileSetViewCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
tests/core/data/tiles/test_tile.cpp (4)
17-64: Consider adding documentation for test scenarios.The test class hierarchy is well-structured, but adding brief documentation explaining the purpose of each test class would improve maintainability.
Consider adding comments like:
// Base class for tile tests providing common initialization template<typename TileSet> class TileTestBase { // ... }; // Tests scenarios where box dimensions are not exact multiples of tile size template<typename TileSet> class TileTestBoxShapeNotMultipleTileSize : public TileTestBase<TileSet> { // ... };
66-75: Consider enhancing the mock class.The TileMock class could be more robust by:
- Adding virtual destructor for proper cleanup
- Implementing mock methods to verify behavior
template<std::size_t dim> class TileMock : public Box<int, dim> { public: virtual ~TileMock() = default; // Add MOCK_METHOD declarations if needed };
85-88: Fix typo in test name and add size validation.The test name has a typo ("cluser" should be "cluster") and could benefit from additional validations.
-TYPED_TEST(TileTestBoxShapeMultipleTileSize, cluserSetSizeIsCorrect) +TYPED_TEST(TileTestBoxShapeMultipleTileSize, clusterSetSizeIsCorrect) { EXPECT_EQ(this->tileSet.size(), std::pow(12, this->dimension)); + EXPECT_GT(this->tileSet.size(), 0); }
91-105: Consider refactoring surface calculation for better readability.The surface calculation logic is complex and could be extracted into a helper method.
template<typename T> double calculateTileSurface(const T& tile) { double surface = 1.0; for (auto d = 0u; d < tile.dimension; ++d) { surface *= (tile.upper[d] - tile.lower[d] + 1); } return surface; } TYPED_TEST(TileTestBoxShapeNotMultipleTileSize, totalTileSetSurfaceIsEqualToBoxSurface) { double totalSurface = 0.0; for (const auto& tile : this->tileSet) { totalSurface += calculateTileSurface(tile); } EXPECT_EQ(totalSurface, this->box.size()); }src/core/data/tiles/tile_set.hpp (2)
19-35: Add class documentation and improve parameter names.The
TileSetViewclass lacks documentation about its ownership semantics and purpose. Additionally, the constructor parameters could be more descriptive.Add class documentation and improve parameter names:
+/** + * A non-owning view into a TileSet's data structure. + * This class provides read/write access to tiles and cells without taking ownership. + * The viewed data must outlive this view instance. + */ template<typename Tile> class TileSetView { public: static auto constexpr dimension = Tile::dimension; TileSetView(Box<int, dimension> const& box, std::array<std::size_t, dimension> const& tile_size, std::array<std::uint32_t, dimension> const& shape, - Tile* tiles, - std::uint32_t tile_nbr, - Tile** cells, - std::array<std::uint32_t, dimension> const& nbr_cells) + Tile* tile_data, + std::uint32_t num_tiles, + Tile** cell_data, + std::array<std::uint32_t, dimension> const& cell_dimensions)
140-144: Document make_view method and consider const overload.The
make_view()method lacks documentation and could benefit from a const overload for read-only views.Add documentation and const overload:
+ /** + * Creates a non-owning view of the tile set's data. + * @return TileSetView providing access to the current state + */ auto make_view() { return TileSetView<Tile>{box_, tile_size_, shape_, tiles_.data(), tiles_.size(), cells_.data(), cells_.shape()}; } + /** + * Creates a const non-owning view of the tile set's data. + * @return TileSetView providing read-only access to the current state + */ + auto make_view() const + { + return TileSetView<const Tile>{box_, tile_size_, shape_, tiles_.data(), + tiles_.size(), cells_.data(), cells_.shape()}; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/core/data/tiles/tile_set.hpp(1 hunks)src/core/utilities/span.hpp(1 hunks)tests/core/data/tiles/test_tile.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/core/data/tiles/tile_set.hpp (1)
Pattern **/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/utilities/span.hpp (1)
Pattern **/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (3)
tests/core/data/tiles/test_tile.cpp (3)
1-14: LGTM! Headers are well organized.
The includes are properly structured with system headers first, followed by project headers, and test framework headers.
189-194: LGTM! Standard test initialization.
The main function properly initializes Google Test and executes all tests.
177-185:
Implement meaningful assertions in getView test.
The test case lacks proper assertions to verify the view's behavior.
Add meaningful assertions to verify:
- View dimensions match expected values
- View shape matches tile set shape
- Tiles in view match original tiles
TYPED_TEST(TileTestBoxShapeNotMultipleTileSize, getView) {
auto view = this->tileSet.make_view();
// Verify view properties
EXPECT_EQ(view.dimension(), this->dimension);
auto viewShape = view.shape();
auto tileSetShape = this->tileSet.shape();
for (auto i = 0u; i < this->dimension; ++i) {
EXPECT_EQ(viewShape[i], tileSetShape[i]);
}
// Verify tiles in view match original tiles
auto viewIt = view.begin();
auto tileSetIt = this->tileSet.begin();
while (viewIt != view.end() && tileSetIt != this->tileSet.end()) {
EXPECT_EQ(*viewIt, *tileSetIt);
++viewIt;
++tileSetIt;
}
}
src/core/utilities/span.hpp
Outdated
| @@ -20,7 +20,7 @@ struct Span | |||
| NO_DISCARD auto& operator[](SIZE i) const { return ptr[i]; } | |||
| NO_DISCARD T const* const& data() const { return ptr; } | |||
| NO_DISCARD T const* const& begin() const { return ptr; } | |||
| NO_DISCARD T* end() const { return ptr + s; } | |||
| NO_DISCARD T const* const& end() const { return ptr + s; } | |||
There was a problem hiding this comment.
Ensure consistency between begin() and end() return types.
The end() method now returns T const* const& while begin() returns T const* const&. While the stronger const-correctness is good, these iterator-pair methods should maintain consistent return types for symmetry and to meet iterator requirements.
Apply this change to maintain consistency:
- NO_DISCARD T const* const& end() const { return ptr + s; }
+ NO_DISCARD T const* const& begin() const { return ptr; }
+ NO_DISCARD T const* const& end() const { return ptr + s; }Additionally, consider documenting the rationale for using const pointer to const by reference return type, as this is a stronger guarantee than typical span implementations.
Committable suggestion skipped: line range outside the PR's diff.
| void make_tiles_() | ||
| { | ||
| auto const size_me = [&](auto dim, auto idx) { | ||
| if (idx == shape_[dim] - 1) | ||
| { | ||
| auto const remain = box_.shape()[dim] % tile_size_[dim]; | ||
| return (remain == 0) ? tile_size_[dim] : remain; | ||
| } | ||
| else | ||
| return tile_size_[dim]; | ||
| }; | ||
|
|
||
| for (auto ix = 0u; ix < shape_[0]; ++ix) | ||
| { | ||
| if constexpr (dimension == 1) | ||
| { | ||
| // -1 because upper is included | ||
| tiles_[ix].lower[0] = box_.lower[0] + ix * tile_size_[0]; | ||
| tiles_[ix].upper[0] = tiles_[ix].lower[0] + size_me(0, ix) - 1; | ||
| } | ||
| else | ||
| { | ||
| for (auto iy = 0u; iy < shape_[1]; ++iy) | ||
| { | ||
| if constexpr (dimension == 2) | ||
| { | ||
| auto const i = ix * shape_[1] + iy; | ||
| tiles_[i].lower[0] = box_.lower[0] + ix * tile_size_[0]; | ||
| tiles_[i].upper[0] = tiles_[i].lower[0] + size_me(0, ix) - 1; | ||
| tiles_[i].lower[1] = box_.lower[1] + iy * tile_size_[1]; | ||
| tiles_[i].upper[1] = tiles_[i].lower[1] + size_me(1, iy) - 1; | ||
| } | ||
| else | ||
| { | ||
| for (auto iz = 0u; iz < shape_[2]; ++iz) | ||
| { | ||
| auto const i = ix * shape_[1] * shape_[2] + shape_[2] * iy + iz; | ||
| tiles_[i].lower[0] = box_.lower[0] + ix * tile_size_[0]; | ||
| tiles_[i].upper[0] = tiles_[i].lower[0] + size_me(0, ix) - 1; | ||
| tiles_[i].lower[1] = box_.lower[1] + iy * tile_size_[1]; | ||
| tiles_[i].upper[1] = tiles_[i].lower[1] + size_me(1, iy) - 1; | ||
| tiles_[i].lower[2] = box_.lower[2] + iz * tile_size_[2]; | ||
| tiles_[i].upper[2] = tiles_[i].lower[2] + size_me(2, iz) - 1; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor dimension-specific tile creation logic.
The make_tiles_() method is complex and hard to maintain due to nested conditionals and manual index calculations. Consider extracting dimension-specific logic into separate methods.
Consider this refactoring approach:
private:
template<std::size_t Dim>
void make_tiles_impl_();
std::size_t calculate_tile_index(std::array<std::uint32_t, dimension> const& coords) const {
if constexpr (dimension == 1) {
return coords[0];
} else if constexpr (dimension == 2) {
return coords[0] * shape_[1] + coords[1];
} else {
return coords[0] * shape_[1] * shape_[2] + shape_[2] * coords[1] + coords[2];
}
}
void make_tiles_() {
if constexpr (dimension == 1) {
make_tiles_impl_<1>();
} else if constexpr (dimension == 2) {
make_tiles_impl_<2>();
} else {
make_tiles_impl_<3>();
}
}![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| for (auto const& cell : tile) | ||
| { | ||
| cells_(cell) = &tile; |
Check warning
Code scanning / CodeQL
Local variable address stored in non-local memory Warning
|
|
||
| TYPED_TEST_SUITE(ParticleTileTest, DimParticleTiles); | ||
|
|
||
| TYPED_TEST(ParticleTileTest, constructs) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
| TYPED_TEST_SUITE(TileTest, DimTiles); | ||
|
|
||
|
|
||
| TYPED_TEST(TileTestBoxShapeNotMultipleTileSize, expectedNbrOfTilesPerDimToCoverTheBox) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
| EXPECT_EQ(shape[i], 14); | ||
| } | ||
|
|
||
| TYPED_TEST(TileTestBoxShapeMultipleTileSize, cluserSetSizeIsCorrect) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
| } | ||
|
|
||
|
|
||
| TYPED_TEST(TileTestBoxShapeNotMultipleTileSize, totalTileSetSurfaceIsEqualToBoxSurface) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
| } | ||
|
|
||
|
|
||
| TYPED_TEST(TileTestBoxShapeNotMultipleTileSize, retrieveTilesFromBoxOverlap) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
| auto incompletes = 0.; | ||
| for (auto const& overlaped : overlapeds) | ||
| { | ||
| auto const& [is_complete, tile] = overlaped; |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| } | ||
|
|
||
|
|
||
| TYPED_TEST(TileTest, cannotCreateTileWithTileSizeBiggerThanBox) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
| } | ||
|
|
||
|
|
||
| TYPED_TEST(TileTestBoxShapeNotMultipleTileSize, canRetrieveTileFromCell) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
tests/core/data/tiles/test_tile.cpp
Outdated
| } | ||
|
|
||
|
|
||
| TYPED_TEST(TileTestBoxShapeNotMultipleTileSize, getView) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
|
Oops thought I did the PR on my fork |
Summary by CodeRabbit
New Features
ParticleTileandParticleTileSetfor enhanced particle management.TileSetclass for managing collections of tiles within defined bounding boxes.TileSetViewclass for improved interaction with tile collections.Bug Fixes
Tests
ParticleTile,TileSet, and tile-related functionalities to ensure robustness.