Skip to content

Adding matrix order=None to matrix classes #495

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

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

Conversation

Transurgeon
Copy link
Contributor

This PR adds an optional order argument to some matrix classes.
This is also the first task of issue #492

@coveralls
Copy link

Coverage Status

coverage: 99.719%. remained the same when pulling e1b00d6 on Transurgeon:adding-matrix-order into 45ccf4b on python-graphblas:main.

@eriknw
Copy link
Member

eriknw commented Aug 26, 2023

Thanks for starting on this @Transurgeon!

I would recommend adding implementation too. Ideally, you should test and verify what you did is needed and works as expected as you add order= arguments. Perhaps this also means adding my_matrix.orientation attribute too, or just using the existing my_matrix.ss.orientation attribute.

I would start with A.T.new(order="columnwise") and test that it works (add tests at the end of graphblas/tests/test_matrix.py).

Also, graphblas/core/utils.py:get_order will be helpful.

@Transurgeon
Copy link
Contributor Author

Transurgeon commented Sep 4, 2023

Hello @eriknw , sorry for the delay on this, I have been finishing up some details for my GSoC project.
I tried to implement the orientation today, however, it is working as planned.
I have added the argument to both Matrix.__new__ and TransposeMatrix.new(). I also called get_order like you recommended, but the test that I have added doesn't seem to pass like I would expect.
I am starting to get lost in the layers of OOP abstraction.
More importantly, I am also having problems when debugging due to the error below, do you have any ideas on how to fix this?
image
P.S Is the expectation to add property.getter/setter for .ss.orientation? I thought that could be a solution but wasn't sure if it was what you wanted.

@eriknw
Copy link
Member

eriknw commented Sep 5, 2023

No worries :) I was also traveling and mostly AFK last week.

I will take a closer look at this PR. As for the error, it looks like you're running from within the python-graphblas/graphblas directory (a builtin library is trying to import select, and it's picking up graphblas/select, which breaks). Try running from python-graphblas directory.

@eriknw
Copy link
Member

eriknw commented Sep 26, 2023

Hey, sorry for letting this sit. I was traveling and on vacation the last two weeks. I should have time to give this attention later this week.

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