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

Change member functions and data names in vector and matrix classes to be more consistent #208

Merged
merged 11 commits into from
Jan 7, 2025

Conversation

pelesh
Copy link
Collaborator

@pelesh pelesh commented Dec 27, 2024

Change names of methods and data members in vector and matrix classes to be more consistent and accurate. Specific changes include:

  • Matrix classes (Sparse, Csr, Csc, Coo)
    • Rename setMatrixData to setDataPointers
    • Rename setNewValues to setValuesPointers
    • Rename updateData to copyData
    • Change input data arrays in copyData to const arrays.
    • Rename owns_*_data_ to owns_*_sparsity_pattern_
  • Vector class
    • Rename deepCopyVectorData to copyDataTo
    • Rename update to copyDataFrom.

Fixes #167

@pelesh pelesh added the enhancement New feature or request label Dec 27, 2024
@pelesh pelesh self-assigned this Dec 27, 2024
@pelesh pelesh force-pushed the matrix-method-naming-dev branch from 9c184a1 to b08a055 Compare December 27, 2024 21:24
@pelesh
Copy link
Collaborator Author

pelesh commented Dec 27, 2024

Rebased to develop.

Copy link
Collaborator

@shakedregev shakedregev left a comment

Choose a reason for hiding this comment

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

Tests are passing, looks good besides the minor comments

resolve/matrix/Coo.cpp Outdated Show resolved Hide resolved
resolve/matrix/Csc.hpp Outdated Show resolved Hide resolved
resolve/vector/Vector.cpp Show resolved Hide resolved
@nkoukpaizan
Copy link
Collaborator

I haven't looked at codes changes, but tests are passing for me as well (tested on Frontier).

resolve/vector/Vector.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@kswirydo kswirydo left a comment

Choose a reason for hiding this comment

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

My main concern is that now we have copyData[To/From] instead of update but at the same time, we kept setDataUpdated which makes things slightly more confusing.

resolve/matrix/Csc.hpp Outdated Show resolved Hide resolved
tests/unit/matrix/LUSOLTests.hpp Outdated Show resolved Hide resolved
tests/unit/matrix/MatrixFactorizationTests.hpp Outdated Show resolved Hide resolved
@pelesh
Copy link
Collaborator Author

pelesh commented Jan 7, 2025

My main concern is that now we have copyData[To/From] instead of update but at the same time, we kept setDataUpdated which makes things slightly more confusing.

I am not sure if this makes things more confusing. The purpose of setDataUpdated is to tell matrix/vector object that its data was changed externally by accessing its raw data pointers. The setDataUpdated is not used in conjunction with copyData* functions because the latter set internal matrix/vector flags to indicate if any object data was changed (updated) during the copy. Perhaps a better name would be setDataChanged?

@pelesh pelesh merged commit 51ea6a9 into develop Jan 7, 2025
4 checks passed
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.

Use consistent naming scheme across matrix and vector classes
4 participants