Skip to content

add function based constructor for band matrix and insertLocalTiles#214

Open
jamabr wants to merge 5 commits intoicl-utk-edu:masterfrom
jamabr:band_matrix_constructor
Open

add function based constructor for band matrix and insertLocalTiles#214
jamabr wants to merge 5 commits intoicl-utk-edu:masterfrom
jamabr:band_matrix_constructor

Conversation

@jamabr
Copy link
Contributor

@jamabr jamabr commented Jan 28, 2025

After some time, I started to synchronize our code to the recent SLATE developments. I was happy to see the progress, which made some of our local adjustments deprecated. For band matrices we require the function based constructor and the insertLocalTiles function. Is this of interest for upstream?

@mgates3
Copy link
Collaborator

mgates3 commented Jan 28, 2025

This looks helpful. I'll look it over. Are there any tests / unit tests that use the new constructor? That would help assure that it is correct.

@jamabr
Copy link
Contributor Author

jamabr commented Jan 28, 2025

This looks helpful. I'll look it over. Are there any tests / unit tests that use the new constructor? That would help assure that it is correct.

Not yet, but I can check how you have implemented the tests for the others and add...

@mgates3
Copy link
Collaborator

mgates3 commented Jan 28, 2025

See slate/unit_test/test_BandMatrix.cc and compare slate/unit_test/test_Matrix.cc. The GitHub CT runs:

slate/unit_test> ./run_tests.py test_BandMatrix

Well, the CT runs the whole unit test suite. That command runs the relevant part of the suite.

@jamabr jamabr marked this pull request as draft January 28, 2025 20:51
@jamabr
Copy link
Contributor Author

jamabr commented Jan 28, 2025

Thanks for pointing me to this, I will add a similar test the next days...

@jamabr jamabr marked this pull request as ready for review February 6, 2025 13:42
@jamabr
Copy link
Contributor Author

jamabr commented Feb 6, 2025

I added a test for the constructor and the new functions, similar to the tests you pointed to from the Matrix class.
Edit: Unfortunately I am not able to identify why the check fails...

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.

2 participants