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

Feature/eckit sparsematrix allocators reorganization, new in-place allocator #148

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

pmaciel
Copy link
Member

@pmaciel pmaciel commented Nov 13, 2024

This add a new "allocator" to the SparseMatrix, which allows building one with direct, non-owned access to a set of Index*, Index* and Scalar*.

It allows low-level control of the data structures, benefiting the following:

  • to provide views, hence promoting parallelism
  • efficient handling of very large cases, by exploiting the internal structure (per-row assembly, non-linear corrections, etc.)
  • creation of utilities that observe and manipulate the structure directly (reordering, manipulation of weights, etc.)

There's no urgency

@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 23 lines in your changes missing coverage. Please review.

Project coverage is 63.75%. Comparing base (9d2807d) to head (e59c541).

Files with missing lines Patch % Lines
src/eckit/linalg/allocator/BufferAllocator.cc 0.00% 11 Missing ⚠️
src/eckit/linalg/allocator/InPlaceAllocator.cc 76.19% 5 Missing ⚠️
src/eckit/linalg/allocator/StandardAllocator.cc 68.75% 5 Missing ⚠️
src/eckit/linalg/SparseMatrix.cc 71.42% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #148   +/-   ##
========================================
  Coverage    63.74%   63.75%           
========================================
  Files         1066     1069    +3     
  Lines        55360    55397   +37     
  Branches      4086     4086           
========================================
+ Hits         35289    35317   +28     
- Misses       20071    20080    +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@pmaciel pmaciel force-pushed the feature/eckit-sparsematrix-allocators branch from 99026bf to e59c541 Compare November 21, 2024 13:13
Copy link

Private downstream CI failed.
Workflow name: private-downstream-ci-hpc
View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/11954033047.

@pmaciel
Copy link
Member Author

pmaciel commented Nov 29, 2024

Say a few more days before merge? I have changes depending on this.

Copy link
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

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

I think this is great! A few points.

minor point: I would change to something like "NonOwningAllocator" or "WrappingAllocator"

more significant point:
When the SparseMatrix is created this way, wrapping existing data, the user of this class should not be allowed to call several methods:

  • SparseMatrix::reserve(rows, cols, nnz)
  • SparseMatrix::load()
  • SparseMatrix::transpose()
  • SparseMatrix::setIdentity()
  • SparseMatrix::prune()

In fact anything that is attempting to change the shape should be forbidden.

One option is to add a boolean flag to SparseMatrix::Shape be set by the Allocator which can be checked if changes to shape are allowed or not. Open to other suggestions.

shape.cols_ = Nc_;

SparseMatrix::Layout layout;
layout.outer_ = reinterpret_cast<decltype(SparseMatrix::Layout::outer_)>(ia_);
Copy link
Member

Choose a reason for hiding this comment

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

I believe a reinterpret_cast or const_cast or any other casting should not be necessary.

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.

3 participants