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.

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

Tiles #922

Closed
wants to merge 10 commits into from
Closed

Tiles #922

Show file tree
Hide file tree
Changes from 8 commits
Commits
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
1 change: 1 addition & 0 deletions res/cmake/test.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ if (test AND ${PHARE_EXEC_LEVEL_MIN} GREATER 0) # 0 = no tests
add_subdirectory(tests/core/numerics/faraday)
add_subdirectory(tests/core/numerics/ohm)
add_subdirectory(tests/core/numerics/ion_updater)
add_subdirectory(tests/core/data/tiles)


add_subdirectory(tests/initializer)
Expand Down
50 changes: 50 additions & 0 deletions src/core/data/particles/particle_tile.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#ifndef PHARE_PARTICLE_TILE_HPP
#define PHARE_PARTICLE_TILE_HPP

#include "core/utilities/box/box.hpp"
#include "core/data/tiles/tile_set.hpp"

namespace PHARE::core
{

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};
};
Comment on lines +10 to +19
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add documentation and consider class semantics.

Several improvements are recommended:

  1. Add documentation explaining the purpose and lifecycle of each particle array type
  2. Consider making this a class with explicit access control
  3. Define move/copy semantics explicitly or delete them
  4. 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};
 };



template<typename ParticleArray>
class ParticleTileSet : public TileSet<ParticleTile<ParticleArray>>
{
public:
static auto constexpr dimension = ParticleArray::dimension;

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;
}
Comment on lines +28 to +46
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix critical issues in select_particles method.

Several issues need to be addressed:

  1. Line 42: domainParticles should be domainParticles_
  2. Line 32: Method name has a typo: overlaped should be overlapped
  3. Performance: Use const bool& for the complete flag in the loop
  4. 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.

Suggested change
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;
}

};
} // namespace PHARE::core

#endif
202 changes: 202 additions & 0 deletions src/core/data/tiles/tile_set.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
#ifndef PHARE_TILE_SET_HPP
#define PHARE_TILE_SET_HPP

#include "core/utilities/box/box.hpp"
#include "core/utilities/types.hpp"
#include "core/def.hpp"
#include "core/data/ndarray/ndarray_vector.hpp"
#include "core/utilities/span.hpp"

#include <array>
#include <utility>
#include <string>

namespace PHARE::core
{
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_;
};
Copy link

Choose a reason for hiding this comment

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

🛠️ 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:

  1. Adding class-level documentation explaining the view semantics
  2. Marking the constructor as noexcept
  3. Adding [[nodiscard]] to getters if you plan to add them
  4. Using gsl::span or std::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 TileSetView

Committable suggestion skipped: line range outside the PR's diff.




template<typename Tile>
class TileSet
{
public:
static auto constexpr dimension = Tile::dimension;

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_();
}
Comment on lines +84 to +103
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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:

  1. Adding validation for zero tile sizes
  2. Documenting the shape calculation formula
  3. 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.



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;
}
Comment on lines +106 to +119
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.


NO_DISCARD auto shape() const { return shape_; }
NO_DISCARD auto size() const { return tiles_.size(); }

NO_DISCARD auto begin() { return tiles_.begin(); }
NO_DISCARD auto begin() const { return tiles_.begin(); }

NO_DISCARD auto end() { return tiles_.end(); }
NO_DISCARD auto end() const { return tiles_.end(); }

NO_DISCARD auto& operator[](std::size_t i) { return tiles_[i]; }
NO_DISCARD auto const& operator[](std::size_t i) const { return tiles_[i]; }

template<typename... Index>
NO_DISCARD auto& at(Index... indexes)
{
return cells_(indexes...);
}


auto make_view()
{
return TileSetView<Tile>{box_, tile_size_, shape_, tiles_.data(),
tiles_.size(), cells_.data(), cells_.shape()};
}

private:
void consistent_tile_size_() const
{
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));
}
}
Comment on lines +149 to +156
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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));
}
}

}

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;
}
}
}
}
}
}
Comment on lines +159 to +207
Copy link

Choose a reason for hiding this comment

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

🛠️ 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>();
        }
    }



//! store the pointer to the tile associated with each cell
void tag_cells_()
{
for (auto& tile : tiles_)
{
for (auto const& cell : tile)
{
cells_(cell) = &tile;

Check warning

Code scanning / CodeQL

Local variable address stored in non-local memory Warning

A stack address (
source
) may be assigned to a non-local variable.
}
}
}


Box<int, dimension> box_;
std::array<std::size_t, dimension> tile_size_;
std::array<std::uint32_t, dimension> shape_;
std::vector<Tile> tiles_;
NdArrayVector<dimension, Tile*> cells_;
};




} // namespace PHARE::core


#endif
1 change: 1 addition & 0 deletions tests/core/data/particles/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ endfunction(_particles_test)

_particles_test(test_main.cpp test-particles)
_particles_test(test_interop.cpp test-particles-interop)
_particles_test(test_particle_tiles.cpp test-particle-tiles)
40 changes: 40 additions & 0 deletions tests/core/data/particles/test_particle_tiles.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"

#include "core/data/particles/particle_array.hpp"
#include "core/data/particles/particle_tile.hpp"
#include "core/data/tiles/tile_set.hpp"

using namespace PHARE::core;

template<std::size_t dimension>
using ParticleTile_t = ParticleTile<ParticleArray<dimension>>;

template<std::size_t dimension>
using TileSet_t = TileSet<ParticleTile_t<dimension>>;

using DimParticleTiles = testing::Types<ParticleTile_t<1>, ParticleTile_t<2>, ParticleTile_t<3>>;

template<typename ParticleTile>
class ParticleTileTest : public ::testing::Test
{
protected:
ParticleTileTest() {}
};

TYPED_TEST_SUITE(ParticleTileTest, DimParticleTiles);
Comment on lines +18 to +25
Copy link

Choose a reason for hiding this comment

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

🛠️ 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)

Check notice

Code scanning / CodeQL

Unused static variable Note test

Static variable gtest_ParticleTileTest_constructs_registered_ is never read.
{
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};
}
Comment on lines +27 to +33
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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
}


int main(int argc, char** argv)
{
::testing::InitGoogleTest(&argc, argv);

return RUN_ALL_TESTS();
}
19 changes: 19 additions & 0 deletions tests/core/data/tiles/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
cmake_minimum_required (VERSION 3.20.1)

project(test-tile)

set(SOURCES test_tile.cpp)

add_executable(${PROJECT_NAME} ${SOURCES})

target_include_directories(${PROJECT_NAME} PRIVATE
${GTEST_INCLUDE_DIRS}
)

target_link_libraries(${PROJECT_NAME} PRIVATE
phare_core
${GTEST_LIBS})

add_no_mpi_phare_test(${PROJECT_NAME} ${CMAKE_CURRENT_BINARY_DIR})


Loading
Loading