-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Default to using Pytorch APIs for monai.transforms
(ETA TBD)
#2231
Comments
as an initial step, the transform should support both tensor and ndarray types, and return value should have the same type as input. |
I had a thought about this. t = Compose([
LoadImage,
ToTensor,
AllOtherTransforms,
...
]) Alternatively, we modify |
because this is a significant feature, how about we create a feature branch for it in the codebase? when it's entirely ready we merge it into dev. otherwise we keep the feature branch up-to-date with the dev branch. all of us can collaborate on the feature branch. |
If I do this as a branch on the Project-MONAI fork, will I have to get an approved code review every time I want to commit? If so, I think I'll just do it as a branch on my fork. |
that's also fine, just two thoughts:
|
Ok. Could we create a branch that has CI/CD testing, but doesn't require code reviews? Potentially also doesn't require the tests to pass before pushing to that particular branch (just to speed things up, we'd have to be careful not to break things of course). |
sounds great, we should do the same for any major new features in the future... |
Great. I can't create branches on the main repository, could you take care of that? |
sure, created |
This is really a necessary change. I just wanted to create a discussion on this since I find my self constantly switching with
I would vote for making this breaking change. MONAI claims to be a "PyTorch-based framework". For all transforms to always expect a torch Tensor and always return a torch Tensor just seems like the right thing to do. Hopefully this makes it into version 0.7.0. |
I've drafted this PR for all transforms, and for the large part, I made the transform ambivalent to torch/numpy input (e.g., Now that I've finished the draft, I've started creating bite-sized PRs to get them into If you check the big PR, there's also functionality for calculating the number of times a conversion is required. Inevitably, some transforms can only operate on torch.Tensor (e.g., |
And regardless of whether the udpated |
@rijobro I get the decision to provide the option to group transforms based on numpy or torch. Is there some long term goal of reimplementing and deprecating all numpy based transforms to torch based transforms? |
I don't see any reason to deprecate them – if possible, we might as well try and support both. Anyway, most will be torch-based. Here's the list from the big PR, the only two numpy transforms are RandHistogramShift (uses np.interp) and Orientation (uses nibabel):
|
monai.transforms
monai.transforms
(ETA TBD)
This is now finalised, more info via
Most IO and performance profiling utility transforms are uncategorised. Thanks for leading this effort @rijobro. |
Is your feature request related to a problem? Please describe.
Based on our discussions, this ticket proposes to implement the MONAI transforms preferably using the Pytorch APIs instead of the current mainly numpy-based solutions.
The text was updated successfully, but these errors were encountered: