Skip to content
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

#6213 Prepend "meta" to MetaTensor.__repr__ and MetaTensor.__str__ for easier identification #6214

Merged
merged 3 commits into from
Mar 21, 2023
Merged

#6213 Prepend "meta" to MetaTensor.__repr__ and MetaTensor.__str__ for easier identification #6214

merged 3 commits into from
Mar 21, 2023

Conversation

MathijsdeBoer
Copy link
Contributor

@MathijsdeBoer MathijsdeBoer commented Mar 21, 2023

Discussed in #6213

Description

Prepends "meta" to the MetaTensor.__repr__ and MetaTensor.__str__ output so printing a MetaTensor does not look the exact same as a regular torch.Tensor.

I don't expect this change to cause any breaks, with me running the risk of invoking xkcd 1172.


1 failure in ./runtests.sh -f -u --net --coverage:

======================================================================
FAIL: test_values (tests.test_tciadataset.TestTciaDataset)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\PythonProjects\MONAI-SaveImageFormatting\tests\test_tciadataset.py", line 72, in test_values
    self.assertTrue(
AssertionError: False is not true

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.

Mathijs de Boer added 2 commits March 21, 2023 15:10
…easier identification

Signed-off-by: Mathijs de Boer <m.deboer-41@umcutrecht.nl>
Signed-off-by: Mathijs de Boer <m.deboer-41@umcutrecht.nl>
@ericspod ericspod requested a review from rijobro March 21, 2023 14:25
@ericspod
Copy link
Member

I believe we had decided to leave it as "tensor" to avoid any changes to appearances, did we have a more specific rationale @rijobro @Nic-Ma @wyli ?

@wyli
Copy link
Contributor

wyli commented Mar 21, 2023

I think pytorch has recently improved this for tensor subclasses:

>>> import torch
>>> class SubTensor(torch.Tensor):
...     pass
... 
>>> a = SubTensor([1.0])
>>> print(a)
SubTensor([1.])

@MathijsdeBoer
Copy link
Contributor Author

MathijsdeBoer commented Mar 21, 2023

@wyli Running pip freeze on the venv with which I'm making my changes contains torch==2.0.0, is this change to the torch.Tensor printing in an in-dev PyTorch version? I ask because I wasn't seeing this behaviour myself.

I've tried seeing what would happen if I would comment out MetaTensor.__repr__ and MetaTensor.__str__:

Python 3.10.10 (tags/v3.10.10:aad5f6a, Feb  7 2023, 17:20:36) [MSC v.1929 64 bit (AMD64)] on win32
>>> import monai
>>> mt = monai.data.MetaTensor([1.])
>>> print(mt)
Metadata
	affine: tensor([[1., 0., 0., 0.],
        [0., 1., 0., 0.],
        [0., 0., 1., 0.],
        [0., 0., 0., 1.]], dtype=torch.float64)
	space: RAS
Applied operations
[]

@wyli
Copy link
Contributor

wyli commented Mar 21, 2023

Hi @MathijsdeBoer my example in the previous comment is not about monai, it's a pytorch demo to show subclassing...

@MathijsdeBoer
Copy link
Contributor Author

@wyli Right, but isn't MetaTensor already a subclass of torch.Tensor? In that case you would expect it to work like in your example, if I'm not mistaken.

@wyli
Copy link
Contributor

wyli commented Mar 21, 2023

these are from the MetaObj...

def __repr__(self) -> str:
"""String representation of class."""
out: str = "\nMetadata\n"
if self.meta is not None:
out += "".join(f"\t{k}: {v}\n" for k, v in self.meta.items())
else:
out += "None"
out += "\nApplied operations\n"
if self.applied_operations is not None:
out += pprint.pformat(self.applied_operations, indent=2, compact=True, width=120)
else:

that's why I think this PR is a good idea..

@wyli
Copy link
Contributor

wyli commented Mar 21, 2023

comment out MetaTensor.__repr__ and MetaTensor.__str__, and change this:
class MetaTensor(MetaObj, torch.Tensor):
to
class MetaTensor(torch.Tensor, MetaObj):
will get the __repr__ resolved to pytorch's implementation :)
but these changes will require more thorough tests

@MathijsdeBoer
Copy link
Contributor Author

Right, order matters. I don't often work with multiple inheritance in my projects, so I didn't think of that! Thanks!

@wyli
Copy link
Contributor

wyli commented Mar 21, 2023

/build

@wyli wyli enabled auto-merge (squash) March 21, 2023 20:20
@wyli wyli merged commit 554b172 into Project-MONAI:dev Mar 21, 2023
@MathijsdeBoer MathijsdeBoer deleted the metatensor-repr branch March 21, 2023 23:16
jak0bw pushed a commit to jak0bw/MONAI that referenced this pull request Mar 28, 2023
…aTensor.__str__` for easier identification (Project-MONAI#6214)

Discussed in Project-MONAI#6213

### Description

Prepends `"meta"` to the `MetaTensor.__repr__` and `MetaTensor.__str__`
output so printing a MetaTensor does not look the exact same as a
regular `torch.Tensor`.

I don't expect this change to cause any breaks, with me running the risk
of invoking [xkcd 1172](https://xkcd.com/1172/).

---

1 failure in `./runtests.sh -f -u --net --coverage`:
```text
======================================================================
FAIL: test_values (tests.test_tciadataset.TestTciaDataset)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\PythonProjects\MONAI-SaveImageFormatting\tests\test_tciadataset.py", line 72, in test_values
    self.assertTrue(
AssertionError: False is not true
```

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [x] New tests added to cover the changes.
- [x] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [x] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [x] In-line docstrings updated.

---------

Signed-off-by: Mathijs de Boer <m.deboer-41@umcutrecht.nl>
jak0bw pushed a commit to jak0bw/MONAI that referenced this pull request Mar 28, 2023
…aTensor.__str__` for easier identification (Project-MONAI#6214)

Discussed in Project-MONAI#6213

### Description

Prepends `"meta"` to the `MetaTensor.__repr__` and `MetaTensor.__str__`
output so printing a MetaTensor does not look the exact same as a
regular `torch.Tensor`.

I don't expect this change to cause any breaks, with me running the risk
of invoking [xkcd 1172](https://xkcd.com/1172/).

---

1 failure in `./runtests.sh -f -u --net --coverage`:
```text
======================================================================
FAIL: test_values (tests.test_tciadataset.TestTciaDataset)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\PythonProjects\MONAI-SaveImageFormatting\tests\test_tciadataset.py", line 72, in test_values
    self.assertTrue(
AssertionError: False is not true
```

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [x] New tests added to cover the changes.
- [x] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [x] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [x] In-line docstrings updated.

---------

Signed-off-by: Mathijs de Boer <m.deboer-41@umcutrecht.nl>
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.

3 participants