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.

Sign up for GitHub

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

OmegaConf.select is relative to the node it's called on #660

Merged
merged 6 commits into from
Apr 5, 2021
Merged

OmegaConf.select is relative to the node it's called on #660

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
78265e4
OmegaConf.select is relative to the node it's called on
omry Apr 5, 2021
87335fd
keys relative by default
omry Apr 5, 2021
f14bc88
absolute key flag
omry Apr 5, 2021
470c7e3
better tests
omry Apr 5, 2021
7684476
minot comment update
omry Apr 5, 2021
d724965
better comment
omry Apr 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions omegaconf/omegaconf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,10 @@ def select(
) -> Any:
try:
try:
if cfg._parent is not None:
# select is relative to the node its called on.
# if called on none-root, prepend dot.
key = f".{key}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The following achieves the same result but is a bit more efficient:

                if absolute_key or key.startswith("."):
                    cfg, key = cfg._resolve_key_and_root(key)

Copy link
Owner Author

@omry omry Apr 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

why is it more efficient? (I am expecting absolute_key to be False in most cases).

Copy link
Collaborator

@odelalleau odelalleau Apr 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In the current code we will often go through this if condition (with absolute_key set to False and key absolute), which triggers the following steps:

  • updating the key string by pre-pending a dot
  • calling _resolve_key_and_root(), which will remove the dot we just added

My suggestion gets rid of both of these. But it probably doesn't matter much given everything else that happens later in most situations, so it's really not a big deal.

cfg, key = cfg._resolve_key_and_root(key)
_root, _last_key, value = cfg._select_impl(
key,
Expand Down
19 changes: 5 additions & 14 deletions tests/test_select.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,11 @@ def test_select_relative_from_nested_node(self, struct: Optional[bool]) -> None:
{"a": {"b": {"c": 10}}, "z": 10},
)
OmegaConf.set_struct(cfg, struct)
assert OmegaConf.select(cfg.a, ".") == {"b": {"c": 10}}
assert OmegaConf.select(cfg.a, "..") == {"a": {"b": {"c": 10}}, "z": 10}
assert OmegaConf.select(cfg.a, "..a") == {"b": {"c": 10}}
assert OmegaConf.select(cfg.a, "..z") == 10
assert OmegaConf.select(cfg.a, "") == {"b": {"c": 10}}
assert OmegaConf.select(cfg.a, "b") == {"c": 10}
assert OmegaConf.select(cfg.a, ".") == {"a": {"b": {"c": 10}}, "z": 10}
assert OmegaConf.select(cfg.a, ".a") == {"b": {"c": 10}}
assert OmegaConf.select(cfg.a, ".z") == 10


@mark.parametrize(
Expand Down Expand Up @@ -261,13 +262,3 @@ def test_select_resolves_interpolation(cfg: Any, key: str, expected: Any) -> Non
OmegaConf.select(cfg, key)
else:
assert OmegaConf.select(cfg, key) == expected


def test_select_relative_from_nested_node() -> None:
cfg = OmegaConf.create(
{"a": {"b": {"c": 10}}, "z": 10},
)
assert OmegaConf.select(cfg.a, ".") == {"b": {"c": 10}}
assert OmegaConf.select(cfg.a, "..") == {"a": {"b": {"c": 10}}, "z": 10}
assert OmegaConf.select(cfg.a, "..a") == {"b": {"c": 10}}
assert OmegaConf.select(cfg.a, "..z") == 10