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

DPI settings from config override the plotting dictionary #807

Closed
ns-rse opened this issue Feb 27, 2024 · 0 comments · Fixed by #808
Closed

DPI settings from config override the plotting dictionary #807

ns-rse opened this issue Feb 27, 2024 · 0 comments · Fixed by #808
Assignees
Labels
Plotting Issues pertaining to the plotting class

Comments

@ns-rse
Copy link
Collaborator

ns-rse commented Feb 27, 2024

The value was added to topostats/default_config.yaml to allow users to over-ride the defaults.

Ideally we want...

if plotting["savefig_dpi"] is None... use the values in topostats/plotting_dictionary.yaml.

else: ... use the values from topostats/default_config.yaml/command line.

@ns-rse ns-rse added the Plotting Issues pertaining to the plotting class label Feb 27, 2024
@ns-rse ns-rse self-assigned this Feb 27, 2024
ns-rse added a commit that referenced this issue Feb 28, 2024
Closes #807

Current process is...

1. `topostats/entry_point.create_parser` : get arguments from commandline via the `process_parser` subparser (includes
   DPI)
2. `topostats/run_topostats.run_topostats()` : loads the dictionary and updates it with any values from `arg`, will
   update `savefig_dpi` with `--savefig-dpi` value.
3. Validate the updated configuration.
4. Load the default `topostats/plotting_dictionary.yaml` to the main `config["plotting"]["plot_dict"]"`. This adds in
   all the image specific settings such as title and importantly DPI values for each image.
5. Validate the `config["plotting"]` configuration.
6. Update the plotting configuration with via `utils.update_plotting_config()`. This adds all the options _not_ defined
   in the `plotting_dictionary` with those from `topostats/default_config.yaml`.

I think this is wrong because we have `plotting_config["plot_dict"][image] = {**options, **main_config}` and so the
options from `**main_config` override those from `**options` as you can not have dictionaries with duplicate keys.

This has been fixed by...

1. Updating the values from `plot_dictionary` with any that exist in the `default_config.yaml` under the `plotting`
   section. Importantly this means that if any are `None` they do not update, logic that already existed in
   `update_config()`.
2. Then adding to each image within `plot_dictionary` any values from the `plotting` section of `default_config.yaml`
   that do _not_ already exist.

This avoids replacing the defaults from `plot_dictionary` with `None` values from the `plotting` section of
`default_config.yaml`.

Tests included which check equality  of dictionaries and that `LOGGER.info()` is included when changes are made to the
DPI.

Other Changes

+ `LOGGER.info()` to `LOGGER.debug()` in `topostats.io.dict_to_hdf5` as I don't think printing the keys is that
  informative.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Plotting Issues pertaining to the plotting class
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant