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

Add a mapping function in transforms.io #7769

Merged
merged 55 commits into from
Aug 28, 2024

Conversation

staydelight
Copy link
Contributor

@staydelight staydelight commented May 14, 2024

Add a function to create a JSON file that maps input and output paths.

Fixes #7557 .

Description

A few sentences describing the changes proposed in this pull request.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • 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.
  • Documentation updated, tested make html command in the docs/ folder.

monai/data/image_reader.py Outdated Show resolved Hide resolved
staydelight and others added 21 commits June 13, 2024 23:00
Add a function to create a JSON file that maps input and output paths.

Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Remove changes unrelated to this issue.

Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Remove changes unrelated to this issue.

Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Remove changes unrelated to this issue.

Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Add code for generating a mapping json file.

Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Change mapping_json_path init way.

Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Fixing unsuccessful checks.

Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Fixes unseccessful ckecks. (if mapping_json_path is not None)

Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
staydelight and others added 4 commits July 15, 2024 16:43
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
pre-commit-ci bot and others added 2 commits August 19, 2024 08:00
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
@staydelight
Copy link
Contributor Author

I had a few minor added comments but otherwise it looks good. If you could please address what I and @KumoLiu have mentioned we can get this through.

@ericspod @KumoLiu All comments have been addressed.If there are any additioanal changes,please let me know.

@ericspod
Copy link
Member

@ericspod @KumoLiu All comments have been addressed.If there are any additioanal changes,please let me know.

Do we not also need a dictionary version of this transform? If not we can approve this as it is now.

@KumoLiu
Copy link
Contributor

KumoLiu commented Aug 23, 2024

Do we not also need a dictionary version of this transform? If not we can approve this as it is now.

Adding a dictionary version would be better since dictionary version pipeline may be more commonly used.

@ericspod
Copy link
Member

Do we not also need a dictionary version of this transform? If not we can approve this as it is now.

Adding a dictionary version would be better since dictionary version pipeline may be more commonly used.

The dictionary version can be something very simple I think, like this:

class WriteFileMappingd(MapTransform):
    def __init__(self, keys: KeysCollection, mapping_file_path: Path | str = "mapping.json", allow_missing_keys: bool = False) -> None:
        super().__init__(keys, allow_missing_keys)
        self.mapping = WriteFileMapping(mapping_file_path)

    def __call__(self, data: Mapping[Hashable, NdarrayOrTensor]) -> dict[Hashable, NdarrayOrTensor]:
        d = dict(data)
        for key in self.key_iterator(d):
            d[key] = self.mapping(d[key])
        return d

This would needed added tests as well.

@KumoLiu
Copy link
Contributor

KumoLiu commented Aug 26, 2024

Hi @staydelight, just a quick note—if you want this new feature included in version 1.4, we only have one or two weeks left to add new features.

staydelight and others added 9 commits August 27, 2024 14:45
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
Signed-off-by: staydelight <kevin295643815697236@gmail.com>
@staydelight
Copy link
Contributor Author

Do we not also need a dictionary version of this transform? If not we can approve this as it is now.

Adding a dictionary version would be better since dictionary version pipeline may be more commonly used.

The dictionary version can be something very simple I think, like this:

class WriteFileMappingd(MapTransform):
    def __init__(self, keys: KeysCollection, mapping_file_path: Path | str = "mapping.json", allow_missing_keys: bool = False) -> None:
        super().__init__(keys, allow_missing_keys)
        self.mapping = WriteFileMapping(mapping_file_path)

    def __call__(self, data: Mapping[Hashable, NdarrayOrTensor]) -> dict[Hashable, NdarrayOrTensor]:
        d = dict(data)
        for key in self.key_iterator(d):
            d[key] = self.mapping(d[key])
        return d

This would needed added tests as well.

I would like to ask if data: Mapping[Hashable, NdarrayOrTensor]) -> dict[Hashable, NdarrayOrTensor] must include Ndarray. Since both the input and output paths require metadata, the input parameter for WriteFileMapping is MetaTensor. In this case, is it possible to use data: Mapping[Hashable, torch.Tensor]) -> dict[Hashable, torch.Tensor] or data: Mapping[Hashable, MetaTensor]) -> dict[Hashable, MetaTensor]?

Also, I've added the test for the dictionary. Please let me know if it's okay. Thank you!

Copy link
Contributor

@KumoLiu KumoLiu left a comment

Choose a reason for hiding this comment

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

Thanks for the quick update, overall looks good to me.
@ericspod @binliunls, do you have any further comments on this one?

Signed-off-by: staydelight <kevin295643815697236@gmail.com>
@KumoLiu
Copy link
Contributor

KumoLiu commented Aug 28, 2024

/build

@KumoLiu KumoLiu enabled auto-merge (squash) August 28, 2024 08:40
@KumoLiu KumoLiu merged commit b6d6d77 into Project-MONAI:dev Aug 28, 2024
28 checks passed
@staydelight staydelight changed the title Add a mapping function in image_reader.py and image_writer.py Add a mapping function in transforms.io Aug 30, 2024
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.

Update the SaveImage transform to support saving input-output mapping.
4 participants