/black

/black

/integration-test

/black

Hi @wyli ,

Thanks for your review.
BTW, there is one logic slightly different from ToTensor transform: if passing a list like: [1, 2, 3, 4], EnsureTensor will convert it to [torch.Tensor(1), torch.Tensor(2), torch.Tensor(3), torch.Tensor(4)], while ToTensor will convert it to torch.Tensor([1, 2, 3, 4]). I prefer to the behavior of EnsureTensor because users may also pass [[1], [2, 3], 4] that can't convert to 1 Tensor array.

Thanks.

Hi @wyli ,

Thanks for your review.
BTW, there is one logic slightly different from ToTensor transform: if passing a list like: [1, 2, 3, 4], EnsureTensor will convert it to [torch.Tensor(1), torch.Tensor(2), torch.Tensor(3), torch.Tensor(4)], while ToTensor will convert it to torch.Tensor([1, 2, 3, 4]). I prefer to the behavior of EnsureTensor because users may also pass [[1], [2, 3], 4] that can't convert to 1 Tensor array.

Thanks.

sure I feel the transform shouldn't change the data structure, but does it mean we shouldn't deprecate ToTensor?

Hi @wyli ,

That's a good point, yes, I think we should keep ToTensor just like the AddChannel and EnsureChannelFirst.
Let me update the PR.

Thanks.

I think we could have more general solutions. We have LoadImage instead of LoadNifti, so we could also have ToType instead of ToTensor. This would take the output type as argument (Numpy, Tensor) etc. and could also have the option to do recursively or not. This would solve your problem, whilst not creating a confusing amount of classes that might leave users unsure as to which they should be using. What do you think?

Hi @rijobro ,

I think the reason we add this EnsureTensor transform is because almost every MONAI program needs to use ToTensor transform in preprocessing and postprocessing now, but ToNumpy is only used in very few cases.
So we want to enhance the transform to convert data to Tensor.

Thanks.

Hi @wyli @rijobro @ericspod ,

So all you guys prefer to EnsureType(dtype: str) now?
If so, I will try to update the PR ASAP and write the decollate document based on it for v0.6 release tomorrow.

Thanks in advance.

Or maybe we can have both ToType and EnsureTensor transforms?
For most cases, users just need to put EnsureTensor in their pre-transforms for collate and post-transforms after decollate.

Thanks.

Hi @wyli @rijobro @ericspod ,

So all you guys prefer to EnsureType(dtype: str) now?
If so, I will try to update the PR ASAP and write the decollate document based on it for v0.6 release tomorrow.

Thanks in advance.

I think we could have a EnsureType(dtype='tensor') API? with an initial and default support for 'tensor'. this should be minor change for the current PR, we can expand it if time permits...

An EnsureType transform and dict version makes sense. We could do a simple version for now that just substitutes in for ToTensor but think about defining it later to convert to and from Pytorch, Numpy, and native Python types as the case allows.

We might want to keep ToTensor as an alias to this since so much code relies on it and it's a common idiom Torchvision and other libraries that follow that pattern.

Hi @wyli @ericspod ,

Plan sounds good, let me try to update this PR today.

Thanks.

Hi @wyli @rijobro @ericspod ,

Thanks for your great suggestions.
I updated the PR according to your comments.
Could you please help review it again?

Thanks in advance.

/black

Hi @wyli ,

Seems the GPU tests are running for 3 hours, is there any issue or expected behavior?

Thanks.

I think it's because I pushed the 0.6.0rc2 tag as requested by Sachi, this has triggered full tests...