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 how we create objects! For example, Vector(float, size=2). #205

Merged
merged 4 commits into from
Apr 12, 2022

Conversation

eriknw
Copy link
Member

@eriknw eriknw commented Apr 11, 2022

I think the changes here make things cleaner.
Next commit will deprecate and change e.g. Matrix.new.

I think the changes here make things cleaner.
Next commit will deprecate and change e.g. `Matrix.new`.
@eriknw
Copy link
Member Author

eriknw commented Apr 11, 2022

@jim22k I want your approval for this. I've proposed this change previously, and I recall you being hesitant. Here's an argument for it: Vector? in IPython will show you the constructor signature, so why not show them the best way to create a new object? Also, previously, users would virtually never need to do Vector(gb_obj, ...), and they would get weird error messages if they tried. Now, the error message will tell them about bad dtype or bad integer for e.g. size parameter.

I know this is kind of a significant deprecation, but, hey, gotta make "Python-GraphBLAS" different somehow, right? Now seems like a good time to make the change, and we can be extra-generous with the deprecation time (and grblas never needs to change).

Also, run slow tests less often in CI
@eriknw
Copy link
Member Author

eriknw commented Apr 11, 2022

This also update v.diag() to use GrB_Matrix_diag. Note that this no longer accepts a dtype, because no coercions are allowed.

Copy link
Member

@jim22k jim22k left a comment

Choose a reason for hiding this comment

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

I like this. It hides the creation of Vector, Matrix, Scalar which holds gb_obj and instead lets those top-level classes be something the user can interact with.

There may be some confusion about why this only creates empty objects and we don't allow crazy-complicated constructors like scipy.sparse.csr_matrix. i.e. they may want to call Vector("FP32", [0, 1, 3], [2.2, 3.3, 4.4]). But given how often a person may want to create an empty object (due to the nature of GraphBLAS), this feels okay. I don't mind a surprise (i.e. must call Vector.from_values() rather than Vector()) being an opportunity for education about the fundamental philosophy of GraphBLAS.

@eriknw
Copy link
Member Author

eriknw commented Apr 12, 2022

I agree with your comment about constructors, and I think it's probably best that we don't try to match scipy.sparse, which has lots of specific forms bundled into a single call. I would like to add type annotations someday, and it would be nice if they were sensible.

@eriknw eriknw merged commit f5d2de1 into python-graphblas:main Apr 12, 2022
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.

None yet

2 participants