Skip to content

gridding init#6

Open
PhilipDeegan wants to merge 1 commit into
masterfrom
pr
Open

gridding init#6
PhilipDeegan wants to merge 1 commit into
masterfrom
pr

Conversation

@PhilipDeegan
Copy link
Copy Markdown
Member

@PhilipDeegan PhilipDeegan commented May 1, 2022

not finished

Summary by CodeRabbit

  • New Features
    • Introduced a memory alignment checker for improved data handling.
    • Added a framework for managing multi-dimensional grids with nested operations.
  • Refactor
    • Simplified container and span components for clearer arithmetic operations.
    • Updated operators to support versatile interactions.
    • Transitioned to a more direct implementation of the vector class.
  • Tests
    • Enhanced benchmarks and test cases to cover new grid and container functionalities, ensuring robustness.
    • Updated assertions in tests to validate overall results instead of individual elements.
    • Introduced a new test function for grid operations.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 19, 2025

Walkthrough

This pull request updates the copyright year to 2025 and introduces a new template function is_aligned for checking pointer alignment in the AVX definitions. A comprehensive Grid class is added for managing multi-dimensional data with nested operations. The Span class is modified to include an additional template parameter and a new utility function for generating spans from multiple containers. The Vector class is simplified by removing unnecessary inheritance, and corresponding test files are updated to utilize the new vector type and grid functionality.

Changes

File(s) Change Summary
inc/mkn/avx/def.hpp Added new template function is_aligned for pointer alignment checks; updated copyright year.
inc/mkn/avx/grid.hpp Introduced new Grid class (with nested NestedGrid) for managing multi-dimensional grids; supports sub-grid operations, element-wise addition and multiplication, and shape validation.
inc/mkn/avx/span.hpp Updated Span class with an additional template parameter; added make_spans to create tuples of spans from multiple containers; updated arithmetic operators.
inc/mkn/avx/types.hpp Updated formatting and removed commented-out lines; no functional changes.
inc/mkn/avx/vector.hpp Simplified Vector by removing the _V_ struct and its inheritance; introduced alias Vector_t using std::vector; removed outdated constructor.
test/bench.cpp, test/test_avx.cpp Updated tests to use Vector_t and make_spans; removed the obsolete vec function; updated span/arr functions to include an extra template parameter; added a new grid test function.
LICENSE.md, inc/mkn/avx.hpp, inc/mkn/avx/array.hpp, inc/mkn/avx/lazy.hpp Updated copyright year to 2025; no functional changes.

Sequence Diagram(s)

sequenceDiagram
    participant Main
    participant Grid
    participant NestedGrid
    participant Assert

    Main->>Grid: Instantiate grid with data & shape
    Grid->>NestedGrid: Request sub-grid via operator>>
    NestedGrid->>Assert: Perform element-wise operations (+=, *=)
    Assert-->>Main: Validate grid operation results
Loading
sequenceDiagram
    participant Bench as Benchmark
    participant Vector as Vector_t
    participant SpanGen as make_spans

    Bench->>Vector: Create vector with initial values
    Vector->>SpanGen: Generate spans from vector(s)
    SpanGen-->>Bench: Return spans for arithmetic operations
Loading

Poem

I'm a coding rabbit, hopping with delight,
New grids and spans make my code feel light.
I align my pointers with a simple check,
Arithmetic changes keep my logic in spec.
With every fresh line, I dance with glee 🥕,
Bounding through updates, as happy as can be!

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (6)
inc/mkn/avx/def.hpp (1)

81-85: Well-implemented alignment check utility

The is_aligned function is a useful utility for checking memory alignment, which is critical for optimal AVX operations. The implementation is simple and effective.

Consider making this function constexpr for compile-time evaluation:

template<typename T, std::uint16_t alignment = 32>
-bool is_aligned(T* t)
+constexpr bool is_aligned(T* t)
{
    return (0 == (((std::size_t)(t)) % alignment));
}

Also, consider adding a null pointer check if that's a valid use case in your codebase:

template<typename T, std::uint16_t alignment = 32>
constexpr bool is_aligned(T* t)
{
+    if (t == nullptr) return true; // or false depending on your requirements
    return (0 == (((std::size_t)(t)) % alignment));
}
inc/mkn/avx/types.hpp (3)

224-224: Remove commented-out code

There's a commented-out line that should be removed as it's no longer needed after the refactoring.

-    // using vector_t    = typename Super::vector_t;

270-275: Clean up debug code and add documentation for new operator

The new operator+=, which allows mixed-type operations, is a useful addition. However, there's a commented-out debug statement that should be removed, and the function lacks documentation.

/**
 * Adds a Type<T1, SIZE> to a Type<T0, SIZE>, storing the result in the first operand.
 * This template allows operations between different but compatible value types.
 */
template<typename T0, typename T1, std::size_t SIZE>
void operator+=(Type<T0, SIZE>& a, Type<T1, SIZE> const& b) noexcept
{
-    // KLOG(INF)<< typeid(Type<T, SIZE>::sub_func_ptr).name();
    a() = Type<T0, SIZE>::add_func_ptr(a(), b());
}

283-288: Add documentation for mixed-type multiplication

Similar to the operator+=, this new mixed-type operator*= would benefit from documentation.

/**
 * Multiplies a Type<T0, SIZE> by a Type<T1, SIZE>, storing the result in the first operand.
 * This template allows operations between different but compatible value types.
 */
template<typename T0, typename T1, std::size_t SIZE>
void operator*=(Type<T0, SIZE>& a, Type<T1, SIZE> const& b) noexcept
{
    a() = Type<T0, SIZE>::mul_func_ptr(a(), b());
}
test/test_avx.cpp (1)

108-157: Comprehensive test cases for the new Grid class

The new grid function provides good test coverage for the Grid class, testing various operations including element-wise operations and dimension manipulation with the >> operator.

Consider adding comments to explain what each test case is verifying, especially since grid operations can be complex:

template<typename T = double>
void grid()
{
    {
+        // Test basic grid addition with same dimensions
        mkn::avx::Vector_t<T> v0(1000, 1), v1(1000, 1);

        mkn::avx::Grid<T, 3> grid0{v0.data(), {10, 10, 10}};
        mkn::avx::Grid<T, 3> grid1{v1.data(), {10, 10, 10}};

        grid0 += grid1;

        mkn::kul::abort_if_not(mkn::kul::float_equals(mkn::kul::math::sum(v0), 2000));
    }

    // Continue adding comments for each test case...
test/bench.cpp (1)

107-109: Benchmark memory considerations.
Declaring multiple vectors with the same size and initial value (2) might skew performance metrics if all operations are homogeneous. Consider varying test data for more realistic benchmarks.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f39f91d and 0c93a7a.

📒 Files selected for processing (7)
  • inc/mkn/avx/def.hpp (1 hunks)
  • inc/mkn/avx/grid.hpp (1 hunks)
  • inc/mkn/avx/span.hpp (9 hunks)
  • inc/mkn/avx/types.hpp (3 hunks)
  • inc/mkn/avx/vector.hpp (1 hunks)
  • test/bench.cpp (6 hunks)
  • test/test_avx.cpp (3 hunks)
🔇 Additional comments (10)
inc/mkn/avx/vector.hpp (1)

43-43: Good refactoring towards simplicity

Simplifying the vector implementation to use a type alias rather than inheritance is a solid architectural improvement. This makes the code more maintainable and easier to understand.

inc/mkn/avx/types.hpp (1)

278-281:

Details

✅ Verification successful

Verify impact of operator behavior change

The operator has been changed from += to -=, which could break existing code. Ensure all callers have been updated accordingly.


🏁 Script executed:

#!/bin/bash
# Search for any code that might still expect operator+= behavior but would now get operator-=
rg -A 3 -B 3 "\+=" --type cpp

Length of output: 7653


Operator Behavior Verification: Confirm Correct Separation of Addition and Subtraction

We verified that the repository now provides two distinct overloads in inc/mkn/avx/types.hpp—one for addition (operator+=) and one for subtraction (operator-=). Our search across the codebase (notably in test suites such as test/test_avx.cpp) confirms that:

  • operator+=(...) remains intact and is used by callers expecting addition.
  • The newly introduced operator-=(...) correctly calls sub_func_ptr and is used where subtraction is required.

There is no evidence that callers expecting addition are inadvertently receiving subtraction behavior. Please double-check that the arithmetic semantics in any non-test caller code align with these separate operator definitions.

test/test_avx.cpp (3)

15-41: Good test coverage for new operator implementations

The updated span test function now correctly tests the new -= and /= operators, which is important given the changes in operators' behavior.


44-74: Updated test aligns with Vector_t refactoring

The arr function has been updated to use the new Vector_t type alias instead of the previous Vector class, which correctly reflects the architectural changes.


161-176:

Details

✅ Verification successful

Verify the correctness of the new template parameters

The main function has been updated to call arr and span with specific template parameters (float, 8, double, 4, etc.). Make sure these parameters match the expectations of the functions and align with the vector sizes used in the AVX implementations.


🏁 Script executed:

#!/bin/bash
# Verify that the AVX implementations support the sizes used in the tests
grep -r "struct Type_<float, 8>" --include="*.hpp" .
grep -r "struct Type_<double, 4>" --include="*.hpp" .

Length of output: 205


AVX Template Parameter Verification Passed

  • Verified that the AVX implementation supports the new template parameters:
    • struct Type_<float, 8> and struct Type_<double, 4> are confirmed in inc/mkn/avx/types.hpp.
  • The calls in main() using arr, grid, and span now match the expected vector sizes in our AVX implementation.
  • No further changes are needed regarding the updated template parameters.
test/bench.cpp (5)

96-98: Good use of spans for multi-operand operations.
Using Vector_t<T> and make_spans for separate operand data is clean and consistent. No correctness issues spotted.


117-119: Single-span usage looks solid.
The one-dimensional data handling for mul_avx_inplace_single is properly adapted to the new Vector_t<T> and make_spans.


129-133: Consistent array-based operation.
Creating a single Vector_t<T> and a span from it aligns well with the new design. This approach is straightforward, clean, and maintainable.


142-143: No issues found with two-span creation.
The move to Vector_t<T> and make_spans is uniform, matches the rest of the codebase, and is easy to follow.


152-153: Consistent single-span adaptation.
Transition to Vector_t<T> combined with make_spans is properly encapsulated, maintaining clear semantics for the test.

Comment thread inc/mkn/avx/types.hpp
Comment on lines 290 to 308
template<typename T, std::size_t SIZE>
void operator*=(Type<T, SIZE>& a, Type<T, SIZE> const& b) noexcept
void operator/=(Type<T, SIZE>& a, Type<T, SIZE> const& b) noexcept
{
a() = Type<T, SIZE>::mul_func_ptr(a(), b());
a() = Type<T, SIZE>::div_func_ptr(a(), b());
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify impact of operator behavior change

Similar to line 278, the operator has been changed from *= to /=, which could break existing code. Ensure all callers have been updated accordingly.


🏁 Script executed:

#!/bin/bash
# Search for any code that might still expect operator*= behavior but would now get operator/=
rg -A 3 -B 3 "\*=" --type cpp

Length of output: 6048


Action Required: Update & Verify Operator Usage

The new /= overload in inc/mkn/avx/types.hpp (lines 290–294) now implements division assignment, whereas the multiplication assignment (*=) overload remains available for multiplication. Our grep search revealed many call sites—especially in test files, span implementations, and grid operations—still using *=. This raises two points for review:

  • Confirm Intended Semantics: Double-check that any call sites which should perform division instead of multiplication have been updated to explicitly use /= rather than the old ambiguous *=.
  • Audit Caller Updates: In files such as test/test_avx.cpp, inc/mkn/avx/span.hpp, and inc/mkn/avx/grid.hpp, ensure that the usage of *= aligns with the intended multiplication behavior. If any of these sites were meant to perform division, update them accordingly.

Once you’ve validated and corrected these call sites where necessary, the operator behavior change won’t inadvertently break existing code.

Comment thread inc/mkn/avx/grid.hpp Outdated
Comment thread inc/mkn/avx/grid.hpp
Comment on lines +87 to +108
while (x_pos <= m_shape[0])
{
std::uint32_t const place = x_pos + y_offset + z_offset;
auto p0 = &this->real().data()[place];
auto const p1 = &that.real().data()[place];
bool const are_aligned = is_aligned(p0) and is_aligned(p1);
if (are_aligned)
{
Span<T> span0{p0, rem_x};
Span<const T> span1{p1, rem_x};
span0 *= span1;
break;
}
else
{
*p0 *= *p1;
}

++x_pos;
--rem_x;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Possible off-by-one in the while loop.
while (x_pos <= m_shape[0]) risks indexing out of bounds if x_pos reaches m_shape[0]. Typically, iteration should proceed while x_pos < m_shape[0] + m_offset[0] when data is zero-based. Double-check the indexing logic to avoid going out of range.

Comment thread inc/mkn/avx/span.hpp Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
inc/mkn/avx/def.hpp (1)

100-105: Consider null pointer edge case in alignment check.
If t is nullptr, (std::size_t)(t) % alignment is zero, which would return true, even though a null pointer is arguably not “aligned” in a practical sense. An optional null check could improve clarity and correctness.

 template<typename T, std::uint16_t alignment = 32>
 bool is_aligned(T* t)
 {
-    return (0 == (((std::size_t)(t)) % alignment));
+    return t != nullptr && (reinterpret_cast<std::uintptr_t>(t) % alignment == 0);
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c93a7a and 33c0159.

📒 Files selected for processing (13)
  • .github/workflows/build_nix.yml (1 hunks)
  • LICENSE.md (1 hunks)
  • inc/mkn/avx.hpp (1 hunks)
  • inc/mkn/avx/array.hpp (1 hunks)
  • inc/mkn/avx/def.hpp (3 hunks)
  • inc/mkn/avx/grid.hpp (1 hunks)
  • inc/mkn/avx/lazy.hpp (1 hunks)
  • inc/mkn/avx/span.hpp (15 hunks)
  • inc/mkn/avx/types.hpp (4 hunks)
  • inc/mkn/avx/vector.hpp (2 hunks)
  • test/bench.cpp (6 hunks)
  • test/test_avx.cpp (4 hunks)
  • test/test_lazy.cpp (8 hunks)
✅ Files skipped from review due to trivial changes (4)
  • inc/mkn/avx.hpp
  • LICENSE.md
  • inc/mkn/avx/array.hpp
  • inc/mkn/avx/lazy.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
  • inc/mkn/avx/types.hpp
  • inc/mkn/avx/span.hpp
  • test/bench.cpp
🔇 Additional comments (46)
inc/mkn/avx/def.hpp (2)

2-2: Year updated to 2025.
No concerns; updating the copyright year is appropriate.


83-94: No impactful change observed.
This appears to be a minor style or whitespace change and does not affect logic.

.github/workflows/build_nix.yml (1)

21-21: Incorporating tests into the build step is beneficial.
Running test alongside bench ensures code correctness while benchmarking. This looks good.

test/test_lazy.cpp (13)

2-7: New includes for logging and vector functionality.
Including "mkn/kul/log.hpp" and "mkn/avx/vector.hpp" suggests logging usage and the new Vector_t type. This is consistent with the refactored vector approach.


15-15: Migrated to mkn::avx::Vector_t<double>.
Switching from manually aligned std::vector to Vector_t<double> improves consistency with the new AVX vector strategy.


20-20: Validation checks the entire container contents.
mkn::kul::abort_if_not(make_span(r) == 4); checks all elements for the expected value. This is more thorough than checking a few positions but could mask partial initialization bugs if everything is always the same. Consider verifying random samples or additional properties if needed.


26-26: Same change to vector type.
Replacing with mkn::avx::Vector_t<double> consistently applies the new approach.


31-31: Entire-container check for multiplication result.
No logical issues seen, but keep in mind broader coverage or random checks if needed.


37-37: Use of Vector_t<double> in fma3() scope.
Maintains consistency with the new vector alias.


46-46: Consistent usage of Vector_t<double> in nested scope.
No issues found.


58-58: fma0() updated with new vector alias.
No concerns; consistent refactoring.


69-70: Retaining approach for multi-vector fma in fma1().
Uses Vector_t<double> and merges the results with multiply-add logic. The changed line includes the type alias for consistency.


78-78: Same usage in fma2().
The uniform approach to Vector_t<double> is maintained.


91-91: Swapped to Vector_t<double> in fn0().
Consistent with the broader refactor.


103-103: fn1() uses Vector_t<double> as well.
No further issues.


114-114: Additional filename logging in main().
This can be helpful for debugging test outputs.

inc/mkn/avx/vector.hpp (3)

2-2: Year updated to 2025.
No concerns; appropriate license update.


42-42: Alias Vector_t introduced.
Using Vector_t is simpler than a custom class hierarchy. This change promotes clarity and consistency.


45-45: No functional change detected.
Likely just a closing brace or whitespace. All good.

test/test_avx.cpp (21)

5-7: Includes look appropriate.
No issues found with these newly introduced includes for math, float, and assert utilities.


9-10: AVX includes.
Brings in the main AVX and Grid headers. No concerns here.


44-44: Template parameter for span().
Defining a default N via mkn::avx::Options::N<T>() is consistent with the rest of the codebase’s pattern.


47-47: Aliasing Vector_t.
Using typename mkn::avx::Vector_t<T> to simplify code is clear and consistent.


56-56: Assertion following addition.
Confirming that a is entirely filled with 5 is a straightforward check for expected behavior.


60-61: Subtraction with scalar.
a -= 2; followed by assert(a == 13); nicely verifies per-element scalar subtraction.


63-64: Subtraction of spans.
No concerns about the element-wise subtraction here.


66-66: Adding a scalar.
a += 2; is consistent with prior usage.


68-69: Division by another span.
a /= b; followed by checking a == 4 confirms the correctness of element-wise division.


73-73: Template parameter for arr().
Mirrors the approach used for span() with default N.


76-77: Array and Vector aliases.
Array_t and Vec definitions simplify usage inside arr().


100-101: Element-wise addition.
a += b; assert(a == 5); is consistent with verifying combined values across all elements.


103-103: Validating multiplication result.
assert(a == 15); ensures correct element-wise multiplication.


137-137: Empty line.
No functional change here.


138-186: New grid() test function.
This function thoroughly exercises the Grid class with offsets, verifying sums and element-wise operations for multi-dimensional data. The tests appear logically sound and comprehensive:

  • Lines 141–150: Checks addition across a 10×10×10 structure.
  • Lines 152–162: Uses S=12 to validate partial offsets and final sums.
  • Lines 164–174: Confirms addition with offset 1 for constant data.
  • Lines 176–186: Combines addition and multiplication on partially offset grids.

No immediate issues noticed.


190-190: Blank line.
No functional change.


193-194: Logging file name.
Printing out the file during runtime can simplify debugging.


195-197: Calling array<T>().
Invoking array<float>() and array<double>() ensures coverage for single-dimensional arrays with different floating-point types.


198-200: Calling arr<T>().
Similar coverage for arr<float>() and arr<double>(), verifying the logic for the templated array-based tests.


201-202: Invoking grid<T>().
Demonstrates usage of the multi-dimensional grid with float and double types.


204-205: Invoking span<T>().
Ensures that the final coverage includes the span functionality for both double and float types.

inc/mkn/avx/grid.hpp (6)

1-30: License header.
All standard clauses look correct and updated to 2025.


44-45: Extending SpanSet<T> into Grid.
This is a natural inheritance approach. Ensure SpanSet<T> methods are wholly compatible with multi-dimensional usage.


69-70: Hard-coded capacity for offsets/shapes.
Here, m_offset and m_shape are declared as std::array<std::size_t, 3>, yet the template accepts dimension as a parameter. For broader reusability, consider either:

  1. Using arrays sized by dimension, or
  2. Enforcing static_assert(dimension <= 3) to avoid confusion.

87-109: Potential off-by-one error in the loop condition.
The condition while (x_pos <= m_shape[0]) might allow x_pos to reach m_shape[0], potentially dereferencing out-of-bounds. Typically, zero-based indexing would require < m_shape[0] instead.

Proposed fix:

- while (x_pos <= m_shape[0])
+ while (x_pos < m_shape[0])

159-161: Offset-based shape trimming.
assert(real().shape()[d] - (2 * m_offset[d]) > 0); is a strict check. If m_offset[d] is large, the shape could become zero or negative, triggering this assertion. Ensure all offsets fit within half the shape for each dimension.


165-190: Grid APIs for data access and manipulation.
Constructors and operators (operator>>) appear consistent and straightforward for partial-grid references. No immediate issues.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
inc/mkn/avx/grid.hpp (2)

155-161: Shape-setting logic is concise.
The _set_shape() function subtracts 2*m_offset[d], implying symmetrical offsets. Ensure this always matches intended usage (e.g., partial sub-grid vs. symmetrical offset).


188-189: Public shape & data members.
While convenient for direct access, consider whether encapsulation might be beneficial. Exposing raw pointers and shapes can risk misuse externally.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 33c0159 and b27e04a.

📒 Files selected for processing (13)
  • .github/workflows/build_nix.yml (1 hunks)
  • LICENSE.md (1 hunks)
  • inc/mkn/avx.hpp (1 hunks)
  • inc/mkn/avx/array.hpp (1 hunks)
  • inc/mkn/avx/def.hpp (3 hunks)
  • inc/mkn/avx/grid.hpp (1 hunks)
  • inc/mkn/avx/lazy.hpp (1 hunks)
  • inc/mkn/avx/span.hpp (15 hunks)
  • inc/mkn/avx/types.hpp (4 hunks)
  • inc/mkn/avx/vector.hpp (2 hunks)
  • test/bench.cpp (6 hunks)
  • test/test_avx.cpp (4 hunks)
  • test/test_lazy.cpp (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
  • LICENSE.md
  • inc/mkn/avx.hpp
  • inc/mkn/avx/def.hpp
  • inc/mkn/avx/lazy.hpp
  • inc/mkn/avx/types.hpp
  • .github/workflows/build_nix.yml
  • inc/mkn/avx/array.hpp
  • test/test_lazy.cpp
  • inc/mkn/avx/span.hpp
🔇 Additional comments (35)
inc/mkn/avx/vector.hpp (3)

2-2: No issues with the updated copyright year.
Everything here remains consistent with the licensing terms.


42-42: Good introduction of the type alias.
Replacing a custom wrapper with using Vector_t provides a simpler and more direct approach to vector usage.


45-45: No further concerns.
The closing brace is properly placed, and the file structure is consistent.

test/test_avx.cpp (15)

5-7: Header inclusions look consistent.
These additional includes from mkn/kul/math.hpp, mkn/kul/float.hpp, and mkn/kul/assert.hpp properly align with the usage patterns in the test file.


9-10: New AVX-related includes are appropriate.
Including mkn/avx.hpp and mkn/avx/grid.hpp ensures that the test suite has access to the new Grid functionality and other AVX utilities.


15-15: Trivial formatting change.
No functional impact.


44-44: Template parameter addition is flexible.
Defining N as mkn::avx::Options::N<T>() for the span test enhances generality for various vector lengths.


47-47: Use of Vector_t is consistent with the refactor.
This transition away from a custom vector class clarifies the code and centralizes allocations.


56-69: Arithmetic operations in span() are correct.
The sequence of addition, multiplication, subtraction, and division checks (asserted final result = 4) reflects valid, intuitive vector operations.


73-74: Extended template signature for arr().
Allowing an additional parameter for dimension or size is consistent with the pattern in span() and fosters code reuse.


77-78: Combining Array and Vector types.
Using both Array_t and Vector_t in this function demonstrates flexible handling of different container types.


100-103: Accurate arithmetic verification.
After incrementing by b, the code checks for 5; then multiplies and checks for 15. This correctly confirms expected vector values.


137-140: New grid() function declaration.
Defining a separate test function for grid-based operations provides clear separation from array/span tests.


141-149: Grid addition test is valid.
Combining two 1000-element vectors with dimension=3 shape ensures correct data indexing. The sum check (2000) appears accurate.


152-162: Second grid block uses consistent logic.
The shape (12×12×12) scenario and sum check are a good test for multi-dimensional alignment and operations.


164-174: Offset-based addition.
Applying partial updates to the grid’s second dimension (offset=1) is properly validated by checking the resulting sum.


176-187: Combined addition and multiplication test.
This block exercises multiple operations at a specific offset range, ensuring code correctness for chained transformations.


194-195: Final test calls to grid<T>() and span<T>().
Integrating these calls seals off the test coverage by verifying multi-dimensional grid operations alongside standard spanning.

test/bench.cpp (6)

96-98: Refactored to use Vector_t and spans in mul_avx.
This approach ensures consistent memory management across the benchmark code.


107-108: Maintaining consistency in mul_avx_inplace.
Replacing the original type with Vector_t further unifies the benchmarks with the refactor.


117-118: Single-span refactor in mul_avx_inplace_single.
Utilizing auto [a] = mkn::avx::make_spans(v0) is a concise, flexible way to handle one vector’s span.


129-130: Multi-element array multiplication.
Casting the data into a single span while using a std::array for the multiplier is a neat demonstration of combined usage.


142-143: Addition in place with Vector_t refactor.
This adheres to the new approach, removing any custom vector inheritance overhead.


152-153: Simplified single-vector addition benchmark.
Using auto [a] is consistent and ensures clarity about the data being tested.

inc/mkn/avx/grid.hpp (11)

1-30: License header inclusion is correct.
No issues with the updated year or license text.


31-33: Header guards and basic setup.
Guard naming is correct and consistent.


34-40: Relevant includes for assertions, math, and span.
Everything appears necessary for the grid’s internal arithmetic and alignment checks.


43-45: Class template design with dimension.
The Grid class builds upon SpanSet<T>, aligning well with the rest of the AVX-based abstractions.


49-65: NestedGrid constructors require dimension alignment.
Constructors properly store offsets and shape. The approach is straightforward.


69-70: Dimension mismatch with fixed-size arrays.
You've hard-coded m_offset and m_shape to a 3-element array, yet the template allows dimension to vary.

Consider either:

  1. Restricting dimension to ≤ 3 and enforcing this at compile time, or
  2. Converting m_offset and m_shape to std::array<std::size_t, dimension> for full generality.

87-87: Potential off-by-one error in the while loop.
while (x_pos <= m_shape[0]) could allow x_pos to become m_shape[0], possibly indexing out of range.

Double-check boundary conditions to avoid an out-of-bounds read or write.


73-114: Element-wise operators ensure shape alignment.
operator*=() and operator+=() both confirm matching dimensions and use aligned spans when possible. Implementation is consistent with your alignment strategy.


165-171: Primary Grid constructor correctness.
Invoking the base SpanSet constructor with product(shape) aligns data length with size for multi-dimensional usage.


173-184: Overloaded operator>> suits slicing.
Returning a NestedGrid from offset or offset array is a flexible approach for sub-grid manipulation.


192-194: File-level structure is complete.
Header guard macros align with the file name, and the file ends properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant