Skip to content

Add a vector class to GridKit#418

Open
pelesh wants to merge 1 commit into
developfrom
slaven/vector
Open

Add a vector class to GridKit#418
pelesh wants to merge 1 commit into
developfrom
slaven/vector

Conversation

@pelesh
Copy link
Copy Markdown
Collaborator

@pelesh pelesh commented May 29, 2026

Description

Add vector class to GridKit to replace std::vector and provide portable data management.

Proposed changes

The LinearAlgebra::Vector class was ported from Re::Solve. It has basic initialization and data management functionality. I can be use as a vector, multivector or vector view class.

Checklist

  • All tests pass.
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows GridKit™ style guidelines.
  • There are unit tests for the new code.
  • The new code is documented.
  • The feature branch is rebased with respect to the target branch.
  • I have updated CHANGELOG.md to reflect the changes in this PR. If this is a minor PR that is part of a larger fix already included in the file, state so.

Further comments

The multivector capability in this class should replace DenseMatrix object in GridKit.

@pelesh pelesh self-assigned this May 29, 2026
@pelesh pelesh added the enhancement New feature or request label May 29, 2026
Copy link
Copy Markdown
Collaborator

@PhilipFackler PhilipFackler left a comment

Choose a reason for hiding this comment

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

For an all-in-one solution like this, I would prefer some revisions as explained in my line comments. None of those is a deal-breaker if this is meant only for "advanced" use-cases, but I still think it would be helpful.

class Vector
{
public:
Vector(IdxT n);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This would need a default constructor for use in the phasor dynamics initialization.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

With default constructor, we would need allocate and/or reallocate methods, as well.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If every bus/component has a constant (hard-coded) size_ then the default constructor is not necessary.

If some components might not know their size until run time, it would make more sense to have a default-constructed vector from the component constructor and then just replace the instance in the allocate method when you know the size (y_ = Vector(n);), rather than "reallocate"ing.

But again, if this will never happen, we don't have to worry about it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If we add option to fault each bus/component then there will be not one but two different hard-coded sizes. One for clear and another for faulted object. I can't think of anything else at the moment. @lukelowry or @abirchfield may have additional use cases where bus/component might need to change its model size.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Everything I can think of would be deterministically sized at initialization. I am struggling to think of a case where we would resize the vector after initialization. I think the only exception I can see that would be tangible to resize after initialization is the network itself or related operations by the SystemModel

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is a common issue with all scientific software that I can think of. You need to compute your workspace before you can allocate it. If you know the size of the vector is 124 a priori, then you can instantiate and allocate it like this:

Vector v(124);
v.allocate(HOST);

Otherwise you need to do something like

Vector v;

// compute size n

v.resize(n);
v.allocate(HOST);

The resize method is there just to give you some flexibility.

case HOST:
if (h_data_ == nullptr)
{
h_data_ = new ScalarT[n_capacity_ * k_];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It doesn't make sense for allocating to be part of the semantics of this function. It certainly wouldn't be obvious to me as a user that this might happen. I'm used to having no implicit memory operations, only explicit (the user has to say so). This should at least be documented if not reconsidered.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, you are right, this should be removed.

* you are doing.
*/
template <typename ScalarT, typename IdxT>
int Vector<ScalarT, IdxT>::setData(ScalarT* data, memory::MemorySpace memspace)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This would make more sense as part of a constructor. An instance should be constructed with ownership and data (if non-owning) for host and device respectively. Ownership should not be allowed to change. And I think the data you're wrapping in the non-owning case shouldn't be allowed to change either. This function wouldn't exist in that case.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I agree, changing ownership after construction is looking for trouble.

This method was experimental and should not be a part of the vector class.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This method was used to get a vector from a multivector. Better solution is to construct non-data-owning vector object instead, or create a separate VectorView object as @lukelowry suggested.

Comment on lines +650 to +652
delete[] h_data_;
h_data_ = new ScalarT[n_capacity_ * k_];
owns_cpu_data_ = true;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This ignores the case where we're already non-owning. See my comment above. If ownership was settled at construction there could be tighter invariants for the rest of these functions.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think it would be good to have allocate and reallocate methods, both refusing to operate on data if not owned by the vector object.

  • allocate should return error message and do nothing if data pointer is not null.
  • reallocate should check if the new size is larger than capacity and if so delete previous and allocate new data.

@pelesh
Copy link
Copy Markdown
Collaborator Author

pelesh commented May 30, 2026

For an all-in-one solution like this, I would prefer some revisions as explained in my line comments. None of those is a deal-breaker if this is meant only for "advanced" use-cases, but I still think it would be helpful.

All of your suggestions are spot on and should be incorporated before this PR is merged.

Copy link
Copy Markdown
Collaborator

@lukelowry lukelowry 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 we need a zombie approach if we do merge this implementation. We should take the best parts of this and graft them onto a more scalable implementation that can isolate the Component in memory (is "closure" the right word here?).

* flags correctly after changing the values.
*/
template <typename ScalarT, typename IdxT>
ScalarT* Vector<ScalarT, IdxT>::getData(memory::MemorySpace memspace)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

getData and its overloads may trigger a memory copy, which I would advise we avoid at all costs once the system is initialized. I see how we could functionally achieve zero-copy with this Vector class, but I do not want to give Components the chance to copy data in this way.

To prevent our future selves from having access to functions like this in simulation-time functions like evaluateResidual, I strongly recommend that we enforce zero-copy de facto after initialization to force us to design scalably. (This is the motivation of VectorView, which I'll draft up so you can get a better picture of what I am imagining)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Zero copy will be enforced.


switch (memspace)
{
case HOST:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this was mentioned in the meeting earlier today, but the Host and Device vocabulary and GPU vocab are not desirable IMHO

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Domain folks do not need to see that. We could implement all functions with HOST memory space as the default, e.g.

int allocate(MemSpace m = HOST);

That way, if you don't specify memory space everything will run on CPU (host). Only if you want to run something on GPU, you would need to add memory space argument, but in that case you would already know what the host and device are.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants