Restore cmap to config; simplify config of cmap/dpi/image format

ns-rse commented 8 months ago

Closes #776

TL;DR

I would really appreciate feedback on the documentation as without clear documentation it is perhaps confusing how the components interact and work and can be modified and getting this as clear as possible will be really helpful. This is the configuration.md file. Ignore the changes to the table at the top and please review and feedback on the section ### Further customisation.

It won't be added to/rendered to the website until this PR is merged so I can't easily provide it as a web-page for review.

Details

Aligns command line and configuration field names with those in matplotlibrc files.
Restores the cmap configuration option to default_config.yaml and introduces savefig_dpi option.
Adds command line options for setting DPI (--savefig-dpi), Colormap (--cmap)and Output file format (--savefig-format).
Expands documentation on how to use custom configuration files or command line options to set the DPI/Colormap/Output format.
Updates the header to topostats.mplstyle to explain how to use it as typically users will have created a copy of the file (after the convenience function topostats create-matplotlibrc was introduced with #773).
To achieve this the dictionary config["plotting"] needed explicitly updating as the update_config() function doesn't update nested configurations (since this is the first PR that introduces command line options that modify any of the values in the nested dictionaries).
Updates options for topostats toposum`` to align withsavefig_format` and adds flag to entry point so output format is consistent.
Updates and expands the configuration documentation explaining how to use these conveniences.

As a consequence quite a few files are touched to ensure that validation and processing functions all have variables that align with those in the configuration.

Testing

If users could test this it would be very much appreciated, if you use the Git installed version something like the following would switch branches and allow you test it.

conda create --name topostats-config # Create and activate a virtual env specific to this
conda activate topostats-config
cd ~/path/to/TopoStats
git pull
git checkout ns-rse/776-config-jigging
pip install -e .
topostats process --output-dir base
topostats create-config test_config.yaml   # Create test_config.yaml to try changing parameters
topostats process --config test_config.yaml --output-dir test1
topostats process  --output-dir test2 --savefig-dpi 10 --cmap rainbow --savefig-format svg
topostats process --config test_config.yaml  --output-dir test3 --savefig-dpi 80 --cmap viridis --savefig-format pdf

Each invocation of topostats process will save output to its own directory (either base, test1, test2 and test3) for comparison. There should be differences between the scan plots under each run, i.e. base v test_config.yaml and saved under test1 and those under test2 and test3 should also differ.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (7dee515) 84.30% compared to head (8bfa700) 84.36%. Report is 5 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #777 +/- ## ========================================== + Coverage 84.30% 84.36% +0.06% ========================================== Files 21 21 Lines 3128 3134 +6 ========================================== + Hits 2637 2644 +7 + Misses 491 490 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

derollins commented 8 months ago

Tested this branch as suggested above, it worked as described and both the updated config file and the new command line options all behave as expected.

Used :

topostats process --output-dir base
topostats create-config -f test_config.yaml  
topostats process --config test_config.yaml --output-dir test1
topostats process  --output-dir test2 --savefig-dpi 10 --cmap rainbow --savefig-format svg
topostats process --config test_config.yaml  --output-dir test3 --savefig-dpi 80 --cmap viridis --savefig-format pdf

Saved the outputs in 4 folders with the different format options.

ns-rse commented 8 months ago

Thanks for checking @derollins and glad the tests work for you.

Did you edit the savefig_dpi/savefig_format/cmap fields in the generated test_config.yaml so that output differed from base output?

Is this the desired functionality for changing these options (i.e. three methods firstly via a custom Matplotlibrc style file; secondly changing a custom_config.yaml; thirdly via command line options)?

Does the documentation make sense? (If you want a rendered view of it you can copy and paste this into here then scroll down to the Further Customisation section).

Is the header to topostats/topostats.mplstyle which is generated by topostats create-matplotlibrc less worrying as it no longer includes DO NOT EDIT and are the instructions contained therein concise and clear?

I've spent 4-5hrs digging through this and making the changes and would like to round it off whilst its fresh in my mind if its not right so I can get back to debugging some other work I'm developing with my TopoStats time next week.

derollins commented 8 months ago

Did you edit the savefig_dpi/savefig_format/cmap fields in the generated test_config.yaml so that output differed from base output?

Yes, changed all three of these in the generated test config file. So it is different from all the other folders.

This is a good improvement in functionality, it's really helpful to have all the most used parameters in one place (the main config file) and accessible from the command line for routine analysis and image generation. It's still helpful to have the matplotlib style file for more detailed changes and for matching presentation or journal style guides etc.

The documentation seems clear, the only small thing I thought of was making it clear that the plotting dictionary is not a file to copy/ generate then edit like the config file and matplotlib style file but a single file to edit. Although I don't know if that would actually overcomplicate.

The new header on the style file is very clear and much less intimidating! I'm no longer scared I will break everything haha.

Everything seems to be working how it should and this is a really helpful change.

ns-rse commented 8 months ago

Thanks for the clarification @derollins

What is most used might change between users and what they are trying to achieve. I've always seen TopoStats as a batch processing tool that produces standardised output for work in progress and expected that where bespoke images need customisation they would be plotted outside of this batch processing framework.

Glad you find the changes useful.

I'll await feedback/approval from other users before merging.

derollins commented 8 months ago

What is most used might change between users and what they are trying to achieve. I've always seen TopoStats as a batch processing tool that produces standardised output for work in progress and expected that where bespoke images need customisation they would be plotted outside of this batch processing framework.

This is true, hopefully the upcoming training sessions may help identify the needs of the 'general' users which may feedback to usability development moving forward.

ns-rse commented 8 months ago

Would be great if we could get a few more eyes on this to checkout that it works as people would like it to please.

ns-rse commented 8 months ago

Thanks for the feedback @MaxGamill-Sheffield

I hadn't thought about None being listed in the completion_message() for DPI/Output Format/cmap and appreciate this would be confusing so thanks for highlighting that. The solution I've gone for (removing the additions that reported these) is different from that suggested (update the config dictionary with parameters from mpl.rcParams early in processing). My reasoning being...

Previously we didn't report these, no one has ever asked to see them in the logging output.
We write configuration options to YAML file via write_yaml() at the end of processing. Its a verbatim copy of that which was used (either user specified or default_config.yaml)and it contains the settings used. If a user didn't specify DPI/cmap/format I'm not sure we should alter this. It could be argued it is useful to provide them but then that would also require writing all other configuration/plotting options to be consistent should the default values ever change in the future.

Currently only a handful of parameters are read from topostats.mplstyle and this is conditional on whether any of these are being over-ridden or not when instantiating the Images() class. Currently we

plottingfuncs.Images() > load_mplstyle() > Set DPI/cmap/format based on arguments to Images()

It is certainly possible to change this process as suggested and..

load_mplstyle() > Update DPI/cmap/format > plottingfuncs.Images()

...but that is a larger amount of work to undertake and introduces scope drift to this PR. If it is desirable to report this information in logging and/or ensure the configuration file that is written contains the default parameters from topostats.mplstyle then we can address that as a separate issue.

One challenge there might be is that as TopoStats evolves the configuration files will become dated (e.g. new options may be added, some may be removed, defaults might change). That said specific versions of TopoStats should always work with an associated configuration file, so perhaps we need to give this more consideration and include TopoStats versions in configs too to ensure results are reproducible. :thinking: :exploding_head:

AFM-SPM / TopoStats