Skip to content

replace "image" and "label" with CommonKeys.IMAGE and CommonKeys.LABEL#3605

Closed
rijobro wants to merge 11 commits intoProject-MONAI:devfrom
rijobro:enum_image_meta_dict
Closed

replace "image" and "label" with CommonKeys.IMAGE and CommonKeys.LABEL#3605
rijobro wants to merge 11 commits intoProject-MONAI:devfrom
rijobro:enum_image_meta_dict

Conversation

@rijobro
Copy link
Copy Markdown
Contributor

@rijobro rijobro commented Jan 7, 2022

Fixes #3603.

This prepares the codebase for tracking meta data.

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • 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.

@rijobro rijobro requested review from Nic-Ma and wyli January 7, 2022 16:44
@rijobro
merge
2252ef0 
Signed-off-by: Richard Brown <[email protected]>
@rijobro rijobro force-pushed the enum_image_meta_dict branch from 6b5839a to 2252ef0 Compare January 12, 2022 14:17
wyli
wyli reviewed Jan 13, 2022
View reviewed changes
Copy link
Copy Markdown
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.

I feel DictPostFixes.META is quite useful for linking data and metadata. not sure about CommonKeys.IMAGE and CommonKeys.LABEL, the "image" is more intuitive, and we shouldn't rely on these for data typing, as it will introduce framework-like contraints... cc @ericspod @Nic-Ma

@rijobro
add missing imports
7a1a67c 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
fix import order
478bd0b 
Signed-off-by: Richard Brown <[email protected]>
@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Jan 13, 2022

Externally, users can still use "image", I just thought that internally we should avoid using "magic" variables where possible which means converting "image" to CommonKeys.IMAGE etc.

I can revert this component of the PR if you would like.

@ericspod
Copy link
Copy Markdown
Member

ericspod commented Jan 13, 2022

In general we shouldn't have magic strings to avoid issues with typos or using inconsistent names in places, our code should exhibit what we believe to be good practice. Users can indeed still use string literals if they want so they aren't constrained to using our constants. It is more verbose this way but I don't feel it's unintuitive, the names of our constants should be as descriptive as the string literals they replace.

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Jan 17, 2022

the names of our constants should be as descriptive as the string literals they replace.

So how about renaming CommonKeys.Image to DefaultKeyStr.Image? or how to make sure we don't give an impression that the keys must be some object/class defined by MONAI? The keys can be any Hashable...

also, would be great to have a separate PR to introduce DictPostFixes, looks like we all happy with it.

@rijobro
Copy link
Copy Markdown
Contributor Author

rijobro commented Jan 17, 2022 •
edited
Loading

Ok, this PR has been updated to only include CommonKeys changes. Will open another for DictPostFixes.

@rijobro rijobro changed the title remove "image", "label", "image_meta_dict" and "label_meta_dict" replace "image" and "label" with CommonKeys.IMAGE and CommonKeys.LABEL Jan 17, 2022
ericspod
ericspod reviewed Jan 17, 2022
View reviewed changes
tests/test_deepgrow_transforms.py
"image": np.arange(9).reshape((1, 1, 3, 3)),
"image_meta_dict": {
CommonKeys.IMAGE: np.arange(9).reshape((1, 1, 3, 3)),
f"{CommonKeys.IMAGE}_meta_dict": {
Copy link
Copy Markdown
Member

@ericspod ericspod Jan 17, 2022

Choose a reason for hiding this comment

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

The "_meta_dict" part is now like a magic string, should we have that stored in CommonKeys and have f"{CommonKeys.Image}_{CommonKeys.MetaDict}"?

Copy link
Copy Markdown
Contributor Author

@rijobro rijobro Jan 17, 2022

Choose a reason for hiding this comment

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

see #3671.

Copy link
Copy Markdown
Member

@ericspod ericspod Jan 17, 2022

Choose a reason for hiding this comment

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

Yeah that looks good though we should commit this PR first.

@ericspod
Copy link
Copy Markdown
Member

ericspod commented Jan 17, 2022

I tried going through everything but there's a lot of changes but it looks good what I saw. The only thing I mentioned was using "_meta_dict" as a literal in places. If we instead had CommonKeys.MetaDict then we could make keys like f"{CommonKeys.Image}_{CommonKeys.MetaDict}" or access dictionaries like d[CommonKeys.Image, CommonKeys.MetaDict] = x which will associate x with a pair containing the two key values. If we went with this approach there would be a lot more changes to make to use that syntax so not in this PR.

@rijobro rijobro mentioned this pull request Jan 17, 2022
Merged
1 task
@rijobro rijobro closed this May 19, 2022
@rijobro rijobro deleted the enum_image_meta_dict branch May 19, 2022 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyli wyli wyli left review comments

@ericspod ericspod ericspod approved these changes

@Nic-Ma Nic-Ma Awaiting requested review from Nic-Ma Nic-Ma is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove "image", "label", "image_meta_dict" and "label_meta_dict" from codebase

3 participants

@rijobro @ericspod @wyli