Write modified images from dl1ab in output DL1 files and fix add_config_metadata

[vuillaut]

This PR allows to write the images in all cases, unless specified not to with no-image.

It also modifies the function add_config_metadata to fix corner cases (especially concerning None) and make it more robust to writing, and re-reading of the config.

Fixes #1127.

[codecov]

Codecov Report

Patch coverage: 97.61% and project coverage change: +0.25% 🎉

Comparison is base (a805812) 73.72% compared to head (432d24a) 73.97%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1145      +/-   ##
==========================================
+ Coverage   73.72%   73.97%   +0.25%     
==========================================
  Files         124      124              
  Lines       12590    12647      +57     
==========================================
+ Hits         9282     9356      +74     
+ Misses       3308     3291      -17     

Files Changed	Coverage Δ
lstchain/image/modifier.py	90.05% <ø> (+2.76%)	⬆️
lstchain/io/io.py	77.82% <85.71%> (+0.57%)	⬆️
lstchain/io/config.py	95.45% <100.00%> (+0.58%)	⬆️
lstchain/io/tests/test_config.py	100.00% <100.00%> (ø)
lstchain/io/tests/test_io.py	100.00% <100.00%> (ø)
lstchain/scripts/lstchain_dl1ab.py	75.00% <100.00%> (+4.90%)	⬆️
lstchain/scripts/tests/test_lstchain_scripts.py	100.00% <100.00%> (+0.34%)	⬆️

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

[moralejo]

This is certainly useful - the original reason for not enabling this option from the start was that this would allow to run again dl1ab (possible with another addition of noise) over the dl1ab outputs. That is a bit dangerous. To be safe, we could write in the file a flag indicating the images are modified (and perhaps also with which parameters), and then prevent the files to be dl1ab'ed again with noise tuning (I think we should start from the original ones if another noise level is needed).

[vuillaut]

This is certainly useful - the original reason for not enabling this option from the start was that this would allow to run again dl1ab (possible with another addition of noise) over the dl1ab outputs. That is a bit dangerous. To be safe, we could write in the file a flag indicating the images are modified (and perhaps also with which parameters), and then prevent the files to be dl1ab'ed again with noise tuning (I think we should start from the original ones if another noise level is needed).

Thank you for the feedback.
I have modified the dl1ab script to write the config parameters in the metadata as it was already done in the r0_to_dl1 workflow.
Then we can simply read the config to know if the images have been produced using the image_modifier, and in this case not allow re-modification.

[vuillaut]

Note: I had to modify the add_config_metadata function as well that was unnecessarily complicated and not allowing simple reading of the dumped config. It now uses the more robust json.dumps function while keeping the original intended functionality of dumping some unsupported types such as LazyConfigValue or PosixPath. I have added a unit test.
Since this has nothing to do with the original PR, I can make a separate one if you prefer (and if there is something to discuss about these changes).

[moralejo]

[...] Since this has nothing to do with the original PR, I can make a separate one if you prefer (and if there is something to discuss about these changes).

May be just change the PR title adding a reference to the modifications about the config reading?

[maxnoe]

One test failing:

FAILED lstchain/scripts/tests/test_lstchain_scripts.py::test_dl1ab_on_modified_images - ValueError: Running lstchain_dl1ab failed with return code 1, output: 
 image_modifier configuration: {'increase_psf': True, 'increase_nsb': True}
Modified images are saved in the output file.
Traceback (most recent call last):
  File "/home/runner/micromamba-root/envs/lst-dev/bin/lstchain_dl1ab", line 8, in <module>
    sys.exit(main())
  File "/home/runner/work/cta-lstchain/cta-lstchain/lstchain/scripts/lstchain_dl1ab.py", line 177, in main
    extra_noise_in_dim_pixels = imconfig["extra_noise_in_dim_pixels"]
KeyError: 'extra_noise_in_dim_pixels'
```

[morcuended]

Failing test is unrelated to changes in this PR. I produced again the run summary file with test data and got run types: DATA, PEDCALIB and DATA (it was expecting DATA, ERROR, DATA types for runs 2005, 2006 and 2008, respectively). Did the R0 files in the test data folders change for run 2006?

assert (run_summary_table["run_type"] == ["DATA", "ERROR", "DATA"]).all()

[maxnoe]

Failing test is unrelated to changes in this PR. I produced again the run summary file with test data and got run types: DATA, PEDCALIB and DATA (it was expecting DATA, ERROR, DATA types for runs 2005, 2006 and 2008, respectively). Did the R0 files in the test data folders change for run 2006?

Yes, I added more files for run 2006 for the tests in #1147 , so that's why it now recognizes 2006 (correctly) as a PEDCALIB run

[vuillaut]

Tests are finally passing !

The merge-base changed after approval.

The merge-base changed after approval.

[morcuended]

my approval keeps being dismissed automatically

[vuillaut]

my approval keeps being dismissed automatically

Hu? Why is that? Were the tests not over?

The merge-base changed after approval.

[morcuended]

my approval keeps being dismissed automatically

Hu? Why is that? Were the tests not over?

I approved again after test completion and also happened the same