Skip to content

Add implementation-specific descriptors #343

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

Merged
merged 13 commits into from
Dec 22, 2022

Conversation

eriknw
Copy link
Member

@eriknw eriknw commented Dec 6, 2022

Closes #20.

@jim22k, I have a few questions:

  1. axb_method= or AxB_method=?
  2. What would better names be for descriptors (to show in the recorder)?
  3. Should we bother with better repr for descriptors, and what would that look like?
  4. I documented SuiteSparse descriptor options in graphblas/core/ss/descriptor.py:get_descriptor, but didn't add it to the docs website. I plan to make a new issue for documenting backend-specific descriptors. Sound good?
  5. There are a couple spots where descriptors are ignored b/c they can't be used, because a backend could always have an extended API that does. Should we try to create the descriptor anyway? If we do and it's non-NULL, should we raise (b/c options aren't used)?
  6. Is there anything else I can do to help make reviewing easier? I doubt this needs careful review for every change, but a second pair of eyes for "reasonableness" is always appreciated.

I think this PR covers pretty much everywhere custom descriptors may be used.

Also, I got rid of caching of custom descriptors. We previously did this b/c descriptors weren't cleaned up properly, not for speed (probably). Now they are cleaned up properly.

@eriknw eriknw requested a review from jim22k December 6, 2022 12:05
@coveralls
Copy link

coveralls commented Dec 6, 2022

Coverage Status

Coverage increased (+0.05%) to 99.422% when pulling 1492c8a on eriknw:descriptor_opts into 70a0d17 on python-graphblas:main.

@eriknw
Copy link
Member Author

eriknw commented Dec 22, 2022

  1. axb_method= or AxB_method=?

I decided to normalize everything to lowercase and raise if this causes duplicates. Since descriptor options are keyword arguments, they can potentially overlap with other argument names, and a workaround is to use a capitalized version of the keyword for the descriptor.

  1. There are a couple spots where descriptors are ignored b/c they can't be used, because a backend could always have an extended API that does. Should we try to create the descriptor anyway? If we do and it's non-NULL, should we raise (b/c options aren't used)?

I decided to create the descriptor, then ignore it. This at least ensures that the descriptor options were valid, which will help us transition to when descriptors can be used.

@jim22k, auto-compute of expressions leads to an interesting edge case when it comes to custom descriptors. Suppose an expression is used causing it to auto-compute (and the result is stored as e.g. expr._value). Now suppose you call .new(**opts) on the expression giving it custom descriptors. Currently, we ignore the custom descriptors and return the already-computed expr._value. I think this is okay. If a workflow with a backend requires fine control with custom descriptors, then auto-compute should probably be disabled to avoid this.

I'm going to merge soon--gonna do one final scan and check coverage.

@eriknw eriknw merged commit ae0a232 into python-graphblas:main Dec 22, 2022
@eriknw
Copy link
Member Author

eriknw commented Dec 22, 2022

This is in!

This is a significant addition, and it feels great to close an issue that is 2.5 years old 😃

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.

Add implementation-specific descriptors
3 participants