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.

Sign up for GitHub

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

Jump to bottom

[DAPHNE-#822] Added MatrixMarket write support #832

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[DAPHNE-#822] Added MatrixMarket write support #832

wants to merge 2 commits into from

Conversation

ldirry
Copy link
Contributor

@ldirry ldirry commented Sep 25, 2024

  • Write support for CSRMatrix in coordinate system
  • Write support for DenseMatrix in array format
  • Support for recognizing symmetric, skew-symmetric matrices, and storing them accordingly in .mtx
  • Added testcases, calling readMM, then writeMM and then comparing if the files are still the same

@ldirry ldirry force-pushed the 882-mm-write-support branch 3 times, most recently from 9714923 to bfa0a93 Compare October 2, 2024 22:23
@ldirry
[DAPHNE-daphne-eu#822] Added MatrixMarket write support
e199dcc 
- Write support for CSRMatrix in coordinate system
- Write support for DenseMatrix in array format
- Support for recognizing symmetric, skew-symmetric matrices, and storing them accordingly in .mtx
- Added testcases, calling readMM, then writeMM and then comparing if the files are still the same
@ldirry ldirry force-pushed the 882-mm-write-support branch from bfa0a93 to e199dcc Compare October 2, 2024 22:27
corepointer
corepointer reviewed Oct 7, 2024
View reviewed changes
Copy link
Collaborator

@corepointer corepointer left a comment

Choose a reason for hiding this comment

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

Generally, this is looking good. Maybe in addition to the things I mentioned in the inline comments to the code, you could separate your code into header and cpp file?

src/runtime/local/io/WriteMM.h Outdated
for (size_t j = 0; j < rows; ++j) {
size_t idx = j * cols + i;
if (strcmp(field, MM_REAL_STR) == 0)
f << std::scientific << std::setprecision(13) << values[idx] << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this value be a candidate for an item in our user configuration?

src/runtime/local/io/WriteMM.h Outdated
Comment on lines 229 to 231
if (strcmp(symmetry, MM_GENERAL_STR) == 0) {
nnz = arg->getNumNonZeros();
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I don't fully understand why the computation above is needed if you return getNumNonZeros or fail.

test/api/cli/io/WriteTest.cpp Outdated
#include <string>

const std::string dirPath = "test/api/cli/io/";
const std::string dirPath2 = "test/runtime/local/io/";

bool compareFiles(const std::string &filePath1, const std::string &filePath2) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this one could go into Utils.h? Ideally with a documenting comment (doxygen format) above the function signature ;-)

@ldirry ldirry force-pushed the 882-mm-write-support branch 7 times, most recently from 6e43df2 to 7e7b4fe Compare October 21, 2024 12:50
@ldirry ldirry force-pushed the 882-mm-write-support branch from 7e7b4fe to c150a45 Compare October 21, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corepointer corepointer corepointer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ldirry @corepointer