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

Combining update_config() and merge_mappings() into a single function #984

Open
ns-rse opened this issue Oct 28, 2024 · 0 comments
Open
Labels
IO Input and Output Low Priority

Comments

@ns-rse
Copy link
Collaborator

ns-rse commented Oct 28, 2024

The utils.update_config() and io.merge_mappings() (introduced in #981) do very similar things, recursively update a dictionary.

utils.update.config()

  • Recursively updates a dictionary (config) with values from a second dictionary or Namespace.
  • Converts base_dir and output_dir to Path via convert_path()
def update_config(config: dict, args: dict | Namespace) -> dict:
    """
    Update the configuration with any arguments.

    Parameters
    ----------
    config : dict
        Dictionary of configuration (typically read from YAML file specified with '-c/--config <filename>').
    args : Namespace
        Command line arguments.

    Returns
    -------
    dict
        Dictionary updated with command arguments.
    """
    args = vars(args) if isinstance(args, Namespace) else args

    config_keys = config.keys()
    for arg_key, arg_value in args.items():
        if isinstance(arg_value, dict):
            update_config(config, arg_value)
        else:
            if arg_key in config_keys and arg_value is not None:
                original_value = config[arg_key]
                config[arg_key] = arg_value
                LOGGER.debug(f"Updated config config[{arg_key}] : {original_value} > {arg_value} ")
    if "base_dir" in config.keys():
        config["base_dir"] = convert_path(config["base_dir"])
    if "output_dir" in config.keys():
        config["output_dir"] = convert_path(config["output_dir"])
    return config

io.merge_mappings()

  • Recursively adds mappings if the value is a MutableMapping (i.e. dictionary)
  • If value isn't a MutableMapping its value updates that of the config.
def merge_mappings(map1: MutableMappingType, map2: MutableMappingType) -> MutableMappingType:
    """
    Merge two mappings (dictionaries), with priority given to the second mapping.
    Note: Using a Mapping should make this robust to any mapping type, not just dictionaries. MutableMapping was needed
    as Mapping is not a mutable type, and this function needs to be able to change the dictionaries.
    Parameters
    ----------
    map1 : MutableMapping
        First mapping to merge, with secondary priority.
    map2 : MutableMapping
        Second mapping to merge, with primary priority.
    Returns
    -------
    dict
        Merged dictionary.
    """
    # Iterate over the second mapping
    for key, value in map2.items():
        # If the value is another mapping, then recurse
        if isinstance(value, MutableMapping):
            # If the key is not in the first mapping, add it as an empty dictionary before recursing
            map1[key] = merge_mappings(map1.get(key, {}), value)
        else:
            # Else simply add / override the key value pair
            map1[key] = value
    return map1

It should be possible to combine these into a single function, avoiding duplication and simplifying the code base so
that a single call can be made to combine/update configurations.

@ns-rse ns-rse added IO Input and Output Low Priority labels Oct 28, 2024
@ns-rse ns-rse changed the title Combing update_config() and merge_mappings() into a single function Combining update_config() and merge_mappings() into a single function Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO Input and Output Low Priority
Projects
None yet
Development

No branches or pull requests

1 participant