Add support for partial config files

[SylviaWhittle]

As discussed in the recent TopoStats catchup, the config file is about to explode with parameters. This will result in a rather cumbersome config file for users which will be intimidating.

After fielding a few options to the experimentalists, it was decided that the least bad way forward might be to add support for partial config files, where any missing fields are added in from the default_config.yaml.

This PR is attempting to do just that. Though I'm sure I've forgotten something.

Oh also this PR is entirely type safe (I think?, at least according to mypy!) It brings some type safety to the beginning of run_topostats.py and marks my first foray into generic typing!

[SylviaWhittle]

I'm sure I've probably forgotten to do a chore or add something so please let me know what they are 😅

But the tests pass (so far) so I think that's a good sign.

Always scary editing the early code in TopoStats

[ns-rse]

Nice way of handling the increasingly complex configuration file and I like the way any specified configuration updates the default and then other command line options take precedence over those.¹

I've suggested an alternative perhaps more descriptive name and tried to expand on the docstrings to give users a better understanding of what options will be used where.

It would be worthwhile updating docs/configuration.md with an explanation of how this works and perhaps examples (on the off chance someone reads the documentation!).

I think we'll have to live with topostats create-config including everything as it would add a lot of complex code to specify which options should be written to file.

Footnotes

1. As it happens I spent most of Tuesday going through topostats/entry_point.py and adding all the configuration options as command line arguments to each entry point as part of Remodel entry points and Command Line Interface to use "Swiss Army knife" approach #517. ↩

[ns-rse]

Would it be worthwhile replacing update_config() in favour of the typed merge_mappings()? They are doing the same thing, although update_config() doesn't use the MutableMapping type and also converts path strings to Path().

[SylviaWhittle]

I'm so sorry I didn't realise I had re-written your function. I assumed update_config did something different!

Don't want to tread on toes, so happy to remove merge_mappings in favour of update_config if you like!

[ns-rse]

Don't worry its only code and I'm not possessive about it.

I don't mind which remains bit glad that we converged on the same solution!

Go with whichever you think is best, currently I think that is merge_mappings(), stronger type-hints and clearer docstring.

[SylviaWhittle]

Tried replacing it, but they do slightly different things, in that update_config takes argparse.Namespace objects, while merge_mappings takes only MutableMappings, attempting to replace causes errors with iteration. I tried to fix this by using vars(args) on the input but it still fails, so if it's okay with you, I'd suggest keeping both for now?

[ns-rse]

Ok, keep both but write up an issue to go through and reconcile the two in the future. On the surface they look very similar so it should be possible to write one function.

[ns-rse]

Thanks for adding the docs/configuration.md @SylviaWhittle

I think this is ready to go.

I've noted the possibility of reconciling update_config() and merge_mappings() into a single function in #984.

[ns-rse]

Thanks for adding the docs/configuration.md @SylviaWhittle

I think this is ready to go.

I've noted the possibility of reconciling update_config() and merge_mappings() into a single function in #984.

[SylviaWhittle]

Rebase onto main after #985

[MaxGamill-Sheffield]

Failing test:

FAILED tests/tracing/test_disordered_tracing.py::test_disordered_trace_grain[test height bias: thick curve height weighting outer, strong height bias] - TypeError: Can only insert double* at [4] in {i8*, i8*, i64, i64, double*, [1 x i64], [1 x i64]}: got float*

on MacOS 3.9 & 3.11 (so probably 3.10 too)

Annoyingly I cannot recreate this using the testing function in VSCode but I can using pytest in the terminal directly.

Seems to be due to Skan version 0.12.0. Using the main's pinned version at 0.11.0 solves the failing tests.

I'll work out how to do a cherry pick commit now :)

[SylviaWhittle]

Thank you very much @MaxGamill-Sheffield

[SylviaWhittle]

Windows 🙃

[MaxGamill-Sheffield]

LOVE this, and love that it:

1. can remove practically all files
2. produces the config file output which contains all the added-by-default params on run

Buuut, how does this get to the users as they'll still be blindsided by a thicc config file upon use of the topostats create-config command.

Should we think about adding a topostats create-config --simple to make a reduced version that's not as scary, perhaps with the following keys / values (+ some commonly used by the experimentalists?)

base_dir: ./tests/resources # Directory in which to search for data files
output_dir: ./output # Directory to output results to
cores: 3 # Number of CPU cores to utilise for processing multiple files simultaneously.
loading:
  channel: Height # Channel to pull data from in the data files.
filter:
  run: true # Options : true, false
grains:
  run: true # Options : true, false
grainstats:
  run: true # Options : true, false
disordered_tracing:
  run: true # Options : true, false
nodestats:
  run: true # Options : true, false
ordered_tracing:
  run: true
splining:
  run: true # Options : true, false
plotting:
  run: true # Options : true, false
summary_stats:
  run: true # Whether to make summary plots for output data

Additionally, one issue I have seen is that any fields designated as null but are not null in the default config are overwritten. i.e. modifying the grains.absolute_area_threshold to:
above: [null, null] # above surface [Low, High] in nm^2 (also takes null)
below: [null, null] # below surface [Low, High] in nm^2 (also takes null)
runs and replaces the above values with [300, 3000] in the produced config file.

So at what level does it replace the values? Is it for each iterative key-value or the whole block if a little bit of it is missing?

[MaxGamill-Sheffield]

Windows 3.9 test fails but looks fishy and nothing:

INFO     topostats:io.py:1108 Extracting image from tests\resources\test_image\minicircle_small.topostats
Windows fatal exception: code 0x80000003

...(multi-threading logs)

Thread 0x00000dc8 (most recent call first):
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\threading.py", line 312 in wait
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\multiprocessing\pool.py", line 858 in next
  File "D:\a\TopoStats\TopoStats\topostats\run_topostats.py", line 190 in run_topostats
  File "D:\a\TopoStats\TopoStats\topostats\entry_point.py", line 372 in entry_point
  File "D:\a\TopoStats\TopoStats\tests\test_run_topostats.py", line 100 in test_run_topostats_process_all
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\python.py", line 159 in pytest_pyfunc_call
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_callers.py", line 103 in _multicall
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_manager.py", line 120 in _hookexec
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_hooks.py", line 513 in __call__
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\python.py", line 1627 in runtest
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\runner.py", line 174 in pytest_runtest_call
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_callers.py", line 103 in _multicall
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_manager.py", line 120 in _hookexec
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_hooks.py", line 513 in __call__
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\runner.py", line 242 in <lambda>
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\runner.py", line 341 in from_call
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\runner.py", line 241 in call_and_report
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\runner.py", line 132 in runtestprotocol
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\runner.py", line 113 in pytest_runtest_protocol
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_callers.py", line 103 in _multicall
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_manager.py", line 120 in _hookexec
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_hooks.py", line 513 in __call__
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\main.py", line 362 in pytest_runtestloop
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_callers.py", line 103 in _multicall
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_manager.py", line 120 in _hookexec
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_hooks.py", line 513 in __call__
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\main.py", line 337 in _main
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\main.py", line 283 in wrap_session
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\main.py", line 330 in pytest_cmdline_main
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_callers.py", line 103 in _multicall
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_manager.py", line 120 in _hookexec
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pluggy\_hooks.py", line 513 in __call__
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\config\__init__.py", line 175 in main
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\_pytest\config\__init__.py", line 201 in console_main
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\Scripts\pytest.exe\__main__.py", line 7 in <module>
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\runpy.py", line 87 in _run_code
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\runpy.py", line 197 in _run_module_as_main
Error: Process completed with exit code 1.

but I can't test this on my machine

[SylviaWhittle]

How about this for a simple config?

This is probably something that will be argued about before deciding on a single simple config, so we can always edit it later once there is consensus.

[alicepyne]

Looks brill to me (&max)

…

On 7 Nov 2024, at 15:48, Sylvia Whittle ***@***.***> wrote: How about this for a simple config? image.png (view on web) <https://github.com/user-attachments/assets/8d3e60b1-a992-4571-beb5-30ab743b8763> This is probably something that will be argued about before deciding on a single simple config, so we can always edit it later once there is consensus. — Reply to this email directly, view it on GitHub <#981 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADJSFMOTAQCDVAMTERNQPMDZ7ODT5AVCNFSM6AAAAABQPSXZMSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRSGU3TMNJYGQ>. You are receiving this because you are subscribed to this thread.

[SylviaWhittle]

Looks like the tests pass.

@MaxGamill-Sheffield I was unable to replicate your issue with the null overrides. It appears to work on my end, sent you the details in slack since I don't get notifications on here reliably.