Repository navigation
Add workspace field to cuTENSOR backend. - #318
Conversation
Selects the workspace for the cuTENSOR contraction. Defaults to WORKSPACE_DEFAULT.
| cuTENSOR's workspace preference for contractions, e.g. `cuTENSOR.WORKSPACE_MAX`; `nothing` | ||
| selects `cuTENSOR.WORKSPACE_DEFAULT`. | ||
| """ | ||
| struct cuTENSORBackend{W} <: AbstractBackend |
There was a problem hiding this comment.
should this be a @kwdef struct maybe?
There was a problem hiding this comment.
I don't see why not.
@kwdef struct cuTENSORBackend{W}
workspace::W = nothing
endAlso do we really need to parameterize the type here? I feel like it's unnecessary.
There was a problem hiding this comment.
Yeah I don't know if it makes a huge difference not to. Do we ever intend to dispatch on W elsewhere? That seems dubious to me
There was a problem hiding this comment.
Do the different values of cuTENSOR.WORKSPACE_MAX, cuTENSOR.WORKSPACE_MIN and cuTENSOR.WORKSPACE_DEFAULT belong to different types, or are they different values of a single type? If the latter, and provided this is a simple primitive type that exists in Base and is not defined in CUTENSOR only, we can declare the type of the newly added workspace field and always fill in the default instead of nothing ?
There was a problem hiding this comment.
They're different values of a CEnum
There was a problem hiding this comment.
Is that something that exists somewhere in Base? I can't find it right away.
There was a problem hiding this comment.
It's from this package https://github.com/JuliaInterop/CEnum.jl
There was a problem hiding this comment.
My interpretation of this issue was that a hard dependency would be required to access these enum types.
Another option is to have the workspace field Symbol (with possible values :min, :max and :default) and have a lookup in the extension.
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| selects `cuTENSOR.WORKSPACE_DEFAULT`. | ||
| """ | ||
| @kwdef struct cuTENSORBackend <: AbstractBackend | ||
| workspace = nothing |
There was a problem hiding this comment.
Can we make this a symbol or enum, so the field access is type stable? I think there are only a few options here so probably exhaustive if-else is fine and fast
There was a problem hiding this comment.
I'd advocate for Symbol as the more user-friendly choice. I am not really sure what advantage there is to using an Enum as a proxy for the existing CEnum here.
Afaik there's only three options: WORKSPACE_MIN, WORKSPACE_MAX, and WORKSPACE_DEFAULT, so I suggest :min, :max and :default.
There was a problem hiding this comment.
I don't have a strong opinion, the only reason to have an enum is this properly validates the input options automatically (you can only construct valid options), and that the cenum is only available in the extension so it's hard to properly get that to work. The symbol is definitely also fine, just requires a bit more careful input checking, but feel free to pick whatever you like!
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Small suggestion, otherwise good to go for me
| """ | ||
| struct cuTENSORBackend <: AbstractBackend end | ||
| @kwdef struct cuTENSORBackend <: AbstractBackend | ||
| workspace::Symbol = :default |
There was a problem hiding this comment.
Can we validate at construction time?
Selects the workspace for the cuTENSOR contraction. Defaults to
WORKSPACE_DEFAULT.