Metal fixes#204
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an optional “metal” dependency group for macOS Metal/MPS support and updates sphere lookup call sites to use the name= keyword when calling dipy.data.get_sphere, improving compatibility with updated function signatures.
Changes:
- Added a new
metalextra with PyObjC Metal/MPS frameworks and included it in theallextra. - Updated several
get_sphere(...)call sites inAFQ/tasks/data.pyto passname=explicitly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| setup.cfg | Adds a metal optional extra and wires it into the all extra dependency set. |
| AFQ/tasks/data.py | Switches sphere acquisition to get_sphere(name=...) in several model/tracking code paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| metal = | ||
| pyobjc-framework-Metal>=12.2.1 | ||
| pyobjc-framework-MetalPerformanceShaders>=12.2.1 |
|
|
||
| rumba_fit = rumba_model.fit(data, mask=nib.load(brain_mask).get_fdata()) | ||
| odf = rumba_fit.odf(sphere=get_sphere(tracking_params["sphere"])) | ||
| odf = rumba_fit.odf(sphere=get_sphere(name=tracking_params["sphere"])) |
| cuslines[cu13]==2.2.3 | ||
| onnxruntime-gpu | ||
| metal = | ||
| cuslines[metal]==2.2.3 |
There was a problem hiding this comment.
IIUC, the gpu and metal are mutually exclusive? Given that, should it really be in all as well?
There was a problem hiding this comment.
Actually, for cuslines, they are not mutually exclusive. Neither is currently in all, but I don't think they need to be?
There was a problem hiding this comment.
Oh, you can install cuslines[cu13, metal]? OTOH, as the name implies, maybe they both need to be in all? 😄
There was a problem hiding this comment.
Yes, you can! GPUstreamlines would still work; it is designed to have multiple backends possible, for example, numba, cuda, and webgpu can all be installed at the same time, and then the user selects which one to use. Practically, I am not sure when you would for this specific combo (a mac with an nvidia gpu?). But I am wondering, maybe all should just include everything that isn't hardware-specific? I am going back and forth on this. I generally think of all as referring to all features, not all hardware, and if we add gpu and metal to it, it will, in most cases, just increase the size of the install unnecessarily. On the other hand, increased install size is not that big of a problem.
Add metal package requirements as an optional install. Also, added argument
nametoget_sphere().