Skip to content

Move suitesparse-specific operators to .ss namespace (and deprecate current location). - #366

Merged
eriknw merged 24 commits into
python-graphblas:mainfrom
eriknw:ss_op
Jan 21, 2023
Merged

Move suitesparse-specific operators to .ss namespace (and deprecate current location).#366
eriknw merged 24 commits into
python-graphblas:mainfrom
eriknw:ss_op

Conversation

@eriknw

@eriknw eriknw commented Jan 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Closes #364

Edit: also closes #356

I kinda wish we had done this from the very beginning to better enable support for different implementations.

So far I only moved "positional" operators such as positioni and firstj, b/c I think we can probably make UDFs for the rest.

To enable deprecations, the current code is somewhat complicated. We'll be able to clean it up again once the deprecated methods are removed in eight-ish months.

This ended up required quite a few changes. I relied on having good test coverage to drive what changes need made. It's very possible I missed some corner case somewhere 🤷‍♂️

I think it would be nice if the GraphBLAS spec had positional operators in some way, but it's TBD whether they should be as e.g. BinaryOp or IndexBinaryOp and maybe IndexSemiring.

@coveralls

coveralls commented Jan 8, 2023 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

coverage: 99.482% (-0.009%) from 99.491%
when pulling 5f097e1 on eriknw:ss_op
into 925fb67 on python-graphblas:main.

@eriknw

eriknw commented Jan 11, 2023

Copy link
Copy Markdown
Member Author

I would like us to explore whether we can come up with good vanilla recipes for the aggregators first, last, first_index, last_index, argmin, and argmax. That way, we wouldn't need to move them to .ss. They seem doable at first glance, but they may not be very efficient.

@eriknw

eriknw commented Jan 17, 2023

Copy link
Copy Markdown
Member Author

Before we merge or release this, I want to verify that we can create UDFs with numba to create all the operators we may need in order to support other backends. This exercise will also be useful for #298.

@eriknw

eriknw commented Jan 18, 2023

Copy link
Copy Markdown
Member Author

Alright, I think this is ready if anybody wants to skim through it (CC @jim22k)

@eriknw eriknw mentioned this pull request Jan 18, 2023
Closed
@jim22k

jim22k commented Jan 20, 2023

Copy link
Copy Markdown
Member

A surprising amount of changes needed, but I agree this will help with supporting multiple backends.

@eriknw

eriknw commented Jan 21, 2023

Copy link
Copy Markdown
Member Author

A surprising amount of changes needed

Right?!

but I agree this will help with supporting multiple backends.

Yup. It'll be particularly interesting to support a non-cffi backend. Still plenty to do to get there.

Thanks for reviewing @jim22k! Merging.

@eriknw
eriknw merged commit 3380e04 into python-graphblas:main Jan 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jim22k jim22k jim22k approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

3 participants

@eriknw @coveralls @jim22k