Skip to content

Conversation

@rijobro
Copy link
Contributor

@rijobro rijobro commented Apr 5, 2022

Description

First PR to MetaTensor branch.

As promised, we'll merge to MetaTensor branch via PRs and code reviews. This is the first PR and just adds the object and some tests. MetaObj allows for subclassing np.ndarray in the future should we want to.

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • New tests added to cover the changes.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@rijobro rijobro requested review from Nic-Ma, ericspod and wyli April 5, 2022 17:16
@wyli
Copy link
Contributor

wyli commented Apr 5, 2022

This is non-breaking, I think we should try to merge this into the dev branch directly so that we can potentially get more early feedback... after this PR we can collaborate on the MetaTensor branch at full speed for all the necessary updates.

for the naming of key classes and modules: meta_obj, MetaObj, meta_tensor, MetaTensor, do you agree with the current PR? @ericspod @Nic-Ma @atbenmurray @yiheng-wang-nv (can discuss on Friday as well).

@rijobro
Copy link
Contributor Author

rijobro commented Apr 6, 2022

Should MetaTensor be MetaTensorImage to add in another layer of inheritance and allow for MetaTensorPoints etc?

@rijobro
Copy link
Contributor Author

rijobro commented Apr 6, 2022

@wyli Is there much point merging this into dev? It won't be used until we have some functionality that uses it.

@wyli
Copy link
Contributor

wyli commented Apr 6, 2022

@wyli Is there much point merging this into dev? It won't be used until we have some functionality that uses it.

yes it'll be convenient for creating demos or experimenting with metatensor using pip install monai-weekly or pip install from git+dev branch. I think the MetaTensor branch will be largely broken as we introduce breaking changes in the following weeks/months.

Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rijobro ,

Thanks for your initial cool idea and quick PR.
If plan to merge to dev branch, I have 3 minor concerns:
(1) As this is a very important base classes, I feel we should write very detailed doc-string with example code and explanation for every arg. Refer to transform:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/transforms/transform.py
(2) MetaObj is the base class for Metadata, it should not be aware of any tensor or numpy specific properties.
(2) Let's try to define fewer APIs in the MetaObj base class to keep it simple and easy to extend.

What do you think?

Thanks.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Apr 6, 2022

Hi @vfdev-5 ,

We are trying to extend PyTorch Tensor with medical metadata properties.
Could you please help also take a look at this first PR from the PyTorch team angle?

Thanks in advance.

@vfdev-5
Copy link
Member

vfdev-5 commented Apr 6, 2022

@Nic-Ma thanks for pinging !

I'll check the PR in details a bit later, right now few comments/info (in case if helpful):

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Apr 6, 2022

And as @vfdev-5 mentioned in another conversation, this MetaTensor may have issue with TorchScript?

Thanks.

@rijobro rijobro changed the base branch from MetaTensor to dev April 7, 2022 10:20
rijobro added 4 commits April 7, 2022 16:05
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@rijobro
Copy link
Contributor Author

rijobro commented Apr 7, 2022

Hi, think I've tried to resolve any outstanding issues. I also added testing for pickling, torchscript and amp.

rijobro added 3 commits April 7, 2022 17:54
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@rijobro
Copy link
Contributor Author

rijobro commented Apr 7, 2022

@wyli @Nic-Ma I can't reproduce this error anywhere and I've tried on quite a few different machines (MacOS, Windows, 3xUbuntu). Can any of you reproduce?

@wyli
Copy link
Contributor

wyli commented Apr 7, 2022

@wyli @Nic-Ma I can't reproduce this error anywhere and I've tried on quite a few different machines (MacOS, Windows, 3xUbuntu). Can any of you reproduce?

I can replicate with MacOS py3.8 and with pip install -U -r requirements-dev.txt, let me have a look...

rijobro added 3 commits April 11, 2022 15:54
This reverts commit 840e7df.

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@rijobro
Copy link
Contributor Author

rijobro commented Apr 11, 2022

@wyli let me know when you've bumped the min pytorch version, hopefully this PR will be good to go at that point.

@rijobro
Copy link
Contributor Author

rijobro commented Apr 12, 2022

Hi @vfdev-5 have you had the chance to look this PR over? Would be great to get your insight.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Apr 12, 2022

Hi @vfdev-5 have you had the chance to look this PR over? Would be great to get your insight.

Hi @rijobro ,

I synced with @vfdev-5 offline, he tried to review this PR several times, but is still not clear about the context and initial intent of this PR, could you please help provide some design doc or usage examples to help him better understand it?

Thanks in advance.

@rijobro
Copy link
Contributor Author

rijobro commented Apr 12, 2022

Sure @Nic-Ma. Hi @vfdev-5, we would like to attach our meta data to our images. Looking at the pytorch docs for extending torch.Tensor, ours is similar in concept to the MetadataTensor, but in implementation it more similar to the LoggingTensor example. This is because we subclass torch.Tensor rather than wrap it so that isinstance still works.

Previously, we used dictionaries of data containing images, their metadata and the transforms that have been applied to them, e.g., {"img": torch.Tensor, "img_meta_dict": dict, "img_transforms": list}. Throughout our code, we sometimes hardcode the suffix _meta_dict, and in other places we allow the user to change the default. This gets messy, especially when we have multiple images (e.g., img and seg).

Looking to the future, we would like our transforms to be able to update metadata when they are applied, so we need to first tidy things up, as there isn't scope for adding extra functionality as things stand.

Hence, what would have previously been:

transform = LoadImaged("img", "seg")
data = {"img": some_filename, "seg": some_other_filename}
data = transform(data)
print(data)  #  {"img": ...,  "img_meta_dict": ..., "seg": ..., "seg_meta_dict": ...}

is now

print(data)  # {"img": ..., "seg": ...}

and now "img" has the meta data and affine transformation matrix that can be extracted with:

im = data["img"]
print(im.meta)  # dictionary with meta data
print(im.affine)  # 4x4 or 3x3  torch.Tensor

Now that every image has its attached affine transformation matrix, it should be much easier to make transforms that correctly update it.

We allow users to disable this functionality in case they aren't interested in it, and it is causing them bother for whatever reason.

@vfdev-5
Copy link
Member

vfdev-5 commented Apr 12, 2022

@rijobro thanks a lot for details!

In terms of implementation, I'll check and comment out in the code.

Another point I was wondering as far as I understand tensor subclassing feature but not sure if this point is related to your idea. When subclassing torch tensor with __torch_function__ we have a possibility to route depending on input func whether ops are changing output type (back to torch.Tensor) or keep it as a subclass.
So, in your case looks like any op keep MetaTensor as MetaTensor. For example, if we sum up it we are still in the domain:

t = torch.tensor([1,2,3])
affine = torch.eye(4)
meta = {"some": "info"}
m = MetaTensor(t, affine=affine, meta=meta)
res = m.sum()
print(res)
> tensor(6)
MetaData
	some: info
	affine: tensor([[1., 0., 0., 0.],
        [0., 1., 0., 0.],
        [0., 0., 1., 0.],
        [0., 0., 0., 1.]])

If this is expected and intended behaviour, please ignore this point.

EDIT:
I was also wondering about how would you merge meta data for ops between several MetaTensors. Right now left operand is defining the meta data and this could a bit counter intuitive for commutative ops like add, multiply etc:

t = torch.tensor([1, 2, 3])
affine = torch.eye(4)
meta = {"some": "info"}

m1 = MetaTensor(t, affine=affine, meta=meta)

t = torch.tensor([10, 20, 30])
affine = torch.eye(5)
meta = {"some1": "info2"}

m2 = MetaTensor(t, affine=affine, meta=meta)

r1 = m1 + m2
r2 = m2 + m1
# r1.meta != r2.meta

@wyli
Copy link
Contributor

wyli commented Apr 12, 2022

When subclassing torch tensor with __torch_function__ we have a possibility to route depending on input func whether ops are changing output type (back to torch.Tensor) or keep it as a subclass.

that's a good point, perhaps we need this to differentiate w/o the batch dim for the collate... (the collate related feature is not included in this PR, we can consider this in the follow-ups.)

@wyli
Copy link
Contributor

wyli commented Apr 12, 2022

/build

@rijobro
Copy link
Contributor Author

rijobro commented Apr 12, 2022

@vfdev-5 thanks for the useful input.

The way that we currently have it, yes, if torch.Tensor__torch_function__ would return a torch.Tensor (which is most operations, barring things like __repr__), then our default implementation is to return a MetaTensor.

So MetaTensor([1]) + torch.tensor([1]) will return the type MetaTensor. This is by design, but can be disabled with set_track_meta(False).

Also, yes, things get a bit confusing when combining meta data. But if m1 is CT and m2 is MRI, then what does it really mean to do m1 + m2 – what should the meta data be? Thankfully, I don't think we'll be doing that sort of operation often. If we do it at all, more likely we'll be doing some sort of operation between image and segmentation, in which they should (a) have similar meta data, and (b) the image would normally come first and so should have its meta data propagated.

@rijobro
Copy link
Contributor Author

rijobro commented Apr 12, 2022

@wyli for collating and decollating, I've been messing around with inspect. If the function is torch.stack and the parent function contains collate, then do collation stuff. If the function is torch.unbind and the parent function contains decollate do decollation stuff.

This would be a follow up PR.

What do you think? It's maybe a little hacky. The alternative is to put that logic into the collation and decollation functions, which is what I did in the original PR.

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, it looks good to me, I put some minor docstring updates here rijobro#3

@rijobro rijobro enabled auto-merge (squash) April 12, 2022 17:05
@wyli
Copy link
Contributor

wyli commented Apr 12, 2022

/build

@wyli wyli disabled auto-merge April 12, 2022 17:28
@wyli wyli enabled auto-merge (squash) April 12, 2022 17:28
@wyli wyli merged commit 1880d38 into Project-MONAI:dev Apr 12, 2022
@rijobro rijobro deleted the MetaTensor_1st_PR branch April 12, 2022 20:37
@wyli wyli mentioned this pull request Apr 13, 2022
7 tasks
Can-Zhao added a commit to Can-Zhao/MONAI that referenced this pull request May 10, 2022
Add padding to filter to ensure same size after anti-aliasing

Use replicate padding insteadof zero padding to avoid artifacts for non-zero boundary

Reuse GaussianSmooth

4073 Enhance DynUNet doc-strings (Project-MONAI#4102)

* Fix doc strings error

Signed-off-by: Yiheng Wang <vennw@nvidia.com>

* remove duplicate places

Signed-off-by: Yiheng Wang <vennw@nvidia.com>

4105 drops pt16 support (Project-MONAI#4106)

* update sys req

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

* temp test

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

* update code for torch>=1.7

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

* temp tests

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

* fixes tests

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

* autofix

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

* fixes import

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

* clear cache

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

* update based on comments

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

* remove temp cmd

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

Make `pixelshuffle` scriptable (Project-MONAI#4109)

* Update the existing functionality to comply with the `torchscript.jit.script` function.

Signed-off-by: Ramon Emiliani <ramon@afxmedical.com>

meta tensor (Project-MONAI#4077)

* meta tensor

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>

4084 Add kwargs for `Tensor.to()` in engines (Project-MONAI#4112)

* [DLMED] add kwargs for to() API

Signed-off-by: Nic Ma <nma@nvidia.com>

* [MONAI] python code formatting

Signed-off-by: monai-bot <monai.miccai2019@gmail.com>

* [DLMED] fix typo

Signed-off-by: Nic Ma <nma@nvidia.com>

* [DLMED] fix flake8

Signed-off-by: Nic Ma <nma@nvidia.com>

* [DLMED] update according to comments

Signed-off-by: Nic Ma <nma@nvidia.com>

Co-authored-by: monai-bot <monai.miccai2019@gmail.com>

fixes pytorch version tests (Project-MONAI#4127)

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

update meta tensor api (Project-MONAI#4131)

* update meta tensor api

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

* update based on comments

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

runtests.sh isort (Project-MONAI#4134)

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>

update citation (Project-MONAI#4133)

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

`ToMetaTensor` and `FromMetaTensor` transforms (Project-MONAI#4115)

to and from meta

no skip if before pytorch 1.7 (Project-MONAI#4139)

* no skip if before pytorch 1.7

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>

* fix

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>

* fix

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>

[DLMED] fix file name in meta (Project-MONAI#4145)

Signed-off-by: Nic Ma <nma@nvidia.com>

4116 Add support for advanced args of AMP (Project-MONAI#4132)

* [DLMED] fix typo in bundle scripts

Signed-off-by: Nic Ma <nma@nvidia.com>

* [DLMED] add support for AMP args

Signed-off-by: Nic Ma <nma@nvidia.com>

* [MONAI] python code formatting

Signed-off-by: monai-bot <monai.miccai2019@gmail.com>

* [DLMED] fix flake8

Signed-off-by: Nic Ma <nma@nvidia.com>

Co-authored-by: monai-bot <monai.miccai2019@gmail.com>

New wsireader (Project-MONAI#4147)

`MetaTensor`: collate; decollate; dataset; dataloader; out=; indexing and iterating across batches (Project-MONAI#4137)

`MetaTensor`: collate; decollate; dataset; dataloader; out=; indexing and iterating across batches (Project-MONAI#4137)
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.

6 participants