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

Enhance ExtractDataKeyFromMetaKeyd to work with MetaTensor #7562

Open
KumoLiu opened this issue Mar 20, 2024 · 9 comments
Open

Enhance ExtractDataKeyFromMetaKeyd to work with MetaTensor #7562

KumoLiu opened this issue Mar 20, 2024 · 9 comments
Assignees
Labels
Contribution wanted enhancement New feature or request good first issue Good for newcomers

Comments

@KumoLiu
Copy link
Contributor

KumoLiu commented Mar 20, 2024

Try ExtractDataKeyFromMetaKeyd to try and pull from the metadata of a MetaTensor later to work with image_only=True

@Kent-McPhail
Copy link

I would like to try and tackle this issue. This would be a first issue for me and am just looking for some guidance on completing it

@KumoLiu
Copy link
Contributor Author

KumoLiu commented Jan 9, 2025

Hi @Kent-McPhail, please refer to this contribution guide: https://github.com/Project-MONAI/MONAI/blob/dev/CONTRIBUTING.md

@eliottvalette
Copy link

Hi @KumoLiu,

I’d also like to help with this issue as it aligns well with my goal of practicing for my first contribution. Can multiple people collaborate on a single issue, or can it only be assigned to one person? I read the contribution guide but couldn't find this information.

Thanks!

@eliottvalette
Copy link

I forked the repo and managed to handle MetaTensors, but I'm struggling to understand what needs to be done with image_only. I've tried to understand it by referring to the functioning of LoadImaged, but I wouldn't mind some help.

@aylward
Copy link
Collaborator

aylward commented Jan 11, 2025

It is great if multiple people collaborate on a single issue, there can be multiple assignees, and please keep posting your questions.

Perhaps @ericspod can provide more info on the intent. In general, image_only is passed to a function/transform when it is returning an image and some other data. For example, Affine() returns the tuple (image, affine) if image_only is false, and returns image otherwise...but I am not following Eric's comment in that PR.

@eliottvalette
Copy link

Nice, that’s great news—thank you!

Also, thank you for explaining how image_only works. That’s roughly what I expected, but I was initially surprised that such a parameter would be part of a transformation focused on metadata. I hadn’t realized that when the input is a dictionary (rather than a MetaTensor), the “image” could end up being stored in what we call metadata.

Additionally, I have a question: since the upgraded version of ExtractDataKeyFromMetaKeyd can handle both dictionaries and MetaTensors, should the output always be a dictionary for consistency, or should it return an updated MetaTensor when a MetaTensor is provided? I implemented an inplace parameter in case the second option (returning the updated MetaTensor) is preferred.

@ericspod
Copy link
Member

This issue is related to how things were implemented before we had MetaTensor. When image_only=False The result from LoadImage is a the pair containing the image and dictionary as discussed, but LoadImaged would put the metadata into a key in the resulting dictionary so that one could access that info. ExtractDataKeyFromMetaKeyd takes a value from this metadata dictionary and puts it into the dictionary it returns (the "data" dictionary). What I had discussed was being able to specify a meta_key in the data dictionary which referred to a MetaTensor instead of a dict instance, and the pull values from its meta member (which is a dictionary) instead. This could be useful for some contexts. I realise this is convoluted having just written "dictionary" so many times, but does that make sense?

Whatever you do ExtractDataKeyFromMetaKeyd will always accept and return a dictionary. I don't think we need to copy any tensors as this transform should only be concerned with modifying the data dictionary.

@Kent-McPhail @eliottvalette if you want to collaborate on this change I suggest one of you make a forked repo and grant the other access. If both of you commit to the same branch then a PR from that branch will credit both of you as authors.

@eliottvalette
Copy link

Hi everyone,

Thank you for clarifying the expected behavior. Stop me if I’m wrong, but I understand that the ExtractDataKeyFromMetaKeyd transform is intended to always return a dictionary and not modify the MetaTensor itself. Based on this, I’ll remove the inplace option entirely and ensure the transform consistently returns metadata as a dictionary. If the input is a MetaTensor, the transform will extract and return MetaTensor.meta directly.

I’d still be happy to collaborate with @Kent-McPhail. However, since I haven’t heard back from him and there haven’t been any commits since October 30, I’d like to confirm whether it’s okay to proceed independently. If so, I’ll propose a fork and open a PR. If collaboration becomes possible later, I’d be more than happy to coordinate.

Thanks again for your guidance,
Eliott

@ericspod
Copy link
Member

ExtractDataKeyFromMetaKeyd transform is intended to always return a dictionary and not modify the MetaTensor itself.

Yes this is true but I should restate with an example. This transform, like others that have the "d" suffix, are dictionary transforms which accept dictionaries and inputs and produce them as outputs, both of which represent a set of named values such as an image with its label. If we load an image called "foo" from "foo.nii" the input to LoadImaged will be {"foo": "foo.nii"}. Doing so with image_only=False will produce an output dictionary where "foo" maps to the loaded image tensor and "foo_meta_dict" maps to the metadata dictionary for that image.

ExtractDataKeyFromMetaKeyd allows us to pull a value from "foo_meta_dict" and add it to our data dictionary as a new key. This value can then be used in other transforms. The change that I see to do is to allow this transform to reference a MetaTensor instead of this dictionary and pull a value from its metadata instead.

This is the example using metadata dictionaries:

inputs={"foo": "foo.nii"}  # initial input mapping item name to path

# load the data using LoadImaged
li = LoadImaged(image_only=False)
dat = li(inputs)   # dictionary with keys "foo" (image tensor) and "foo_meta_dict" (metadata dict)

# use transform to pull value "filename_or_obj" in "foo_meta_dict"
e = ExtractDataKeyFromMetaKeyd("filename_or_obj", meta_key="foo_meta_dict")
dat = e(dat)  # now has keys "foo", "foo_meta_dict", and "filename_or_obj"

# verify that the data was pulled correctly from the nested dictionary
assert dat["foo_meta_dict"]["filename_or_obj"] == dat["filename_or_obj"]

Instead I want to be able to refer to the MetaTensor itself and pull the same piece of data from its meta dictionary:

inputs={"foo": "foo.nii"}  # initial input mapping item name to path

# load the data using LoadImaged
li = LoadImaged() 
dat = li(inputs)   # dictionary with keys "foo" (image tensor)

# use transform to pull value "filename_or_obj" in metadata of "foo"
e = ExtractDataKeyFromMetaKeyd("filename_or_obj", meta_key="foo")
dat = e(dat)  # now has keys "foo" and "filename_or_obj"

# verify that the data in the MetaTensor metadata matches what we pulled out
assert dat["foo"].meta["filename_or_obj"] == dat["filename_or_obj"]

I think this is a feasible change to make such that the transform can detect if the meta_key argument references a dictionary or a MetaTensor, then behaves accordingly. If you wanted to start work on this and coordinate later I will leave it up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution wanted enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants