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

Fixing DPI settings #808

Merged
merged 2 commits into from
Mar 5, 2024
Merged

Fixing DPI settings #808

merged 2 commits into from
Mar 5, 2024

Conversation

ns-rse
Copy link
Collaborator

@ns-rse ns-rse commented 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.

This was 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.

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.
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.77%. Comparing base (de3ac4b) to head (ccfbe23).
Report is 63 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #808      +/-   ##
==========================================
+ Coverage   84.73%   84.77%   +0.04%     
==========================================
  Files          21       21              
  Lines        3196     3205       +9     
==========================================
+ Hits         2708     2717       +9     
  Misses        488      488              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@MaxGamill-Sheffield MaxGamill-Sheffield left a comment

Choose a reason for hiding this comment

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

Looks good, and tests well!

I've just added a commit to change the "Options: .." bit in the config file so it no longer talks about "figure".

P.S. thanks for changing the .topostats logger setting too, it was driving me up the wall hahaha

@ns-rse ns-rse merged commit b9212b8 into main Mar 5, 2024
16 checks passed
@ns-rse ns-rse deleted the ns-rse/807-dpi branch March 5, 2024 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DPI settings from config override the plotting dictionary
2 participants