Pydantic model

[zain-sohail]

Please see the config model for complete possible schema. I extracted all parts from mpes and flashloader currently. Though mpes yaml config is not yet updated with the ConfigModel.

To run

from sed.config.config_model import ConfigModel, CoreModel
from sed.core.config import parse_config
config_file = '../sed/config/flash_example_config.yaml'
config = parse_config(config_file)
model = CoreModel(**config["core"])
print(model)
print(model.loader)

model = ConfigModel(**config)
print(model)

config_dict_verified = model.model_dump()

This PR currently won't pass tests as it has config alterations that are breaking. Since creating a Pydantic schema/model, I thought it's good if the dataframe section is better streamlined.

So now there is a columns section. Instead of

  x_column: X  
  y_column: Y
  tof_column: t
  corrected_x: Xm
  ...

we would need

  columns:
    x: X
    y: Y
    tof: t
    corrected_x: Xm
    ...

Another thing to discuss: Whether we do a dict dump after schema verification or use the model itself. I prefer the model, as it makes a nice object oriented way of accessing values.

I won't update the other codebase until we decide if these alterations make sense. A Pydantic model for the updated config is also included here. This can be easily adapted to the config schema in v1, if necessary.

@rettigl please take a look.

[rettigl]

Generally, I find this a good idea, and also support directly using the pydantic models as config items. However, I don't like the idea to put again so many defaults into the code. If we remove all values there, and just make all parameters that are part of the default config as required, we would also get a consistent set, and also can make sure that certain values are available in the modules, no?

[zain-sohail]

Generally, I find this a good idea, and also support directly using the pydantic models as config items. However, I don't like the idea to put again so many defaults into the code. If we remove all values there, and just make all parameters that are part of the default config as required, we would also get a consistent set, and also can make sure that certain values are available in the modules, no?

Makes sense. I don't think there should be no problem in moving the defaults to how it currently is (in v1 branch). I did this because pydantic seems to promote the idea of defaults within model.

I am still not satisfied with how the config and model looks for channels. Will try to think of a better structure.

[coveralls]

Pull Request Test Coverage Report for Build 11456026831

Details

• 454 of 470 (96.6%) changed or added relevant lines in 19 files are covered.
• 10 unchanged lines in 4 files lost coverage.
• Overall coverage increased (+0.1%) to 92.673%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
sed/calibrator/delay.py	21	22	95.45%
sed/calibrator/energy.py	33	34	97.06%
sed/calibrator/momentum.py	14	15	93.33%
sed/loader/sxp/loader.py	3	4	75.0%
sed/core/processor.py	42	54	77.78%

Files with Coverage Reduction	New Missed Lines	%
sed/loader/mirrorutil.py	1	77.48%
tests/calibrator/test_energy.py	2	99.43%
sed/core/processor.py	3	85.57%
sed/calibrator/energy.py	4	92.45%

Totals
Change from base Build 11307718960:	0.1%
Covered Lines:	7538
Relevant Lines:	8134

---

💛 - Coveralls

[rettigl]

@zain-sohail I updated the mode and code so it passes tests and has all necessary fields. There are still some checks that could be added, and some tests for the schema could be added.

[zain-sohail]

@zain-sohail I updated the mode and code so it passes tests and has all necessary fields. There are still some checks that could be added, and some tests for the schema could be added.

Thanks a lot for fixing that.

What checks are you thinking of?

Regarding tests, generally, since Pydantic itself is well tested, we can expect it to do what it is supposed to. Or did you mean some integration testing? But I do find that after this config validation, a lot of checks within the rest code can be removed. Either I can look into it in this PR or we merge this and do that later.

[rettigl]

I thought about tests that the config fits to the model, and fails if additional entries are present, or required missing. The former e g was not working on your version.
Also, further checks regarding the consistency can be added to the config model. Some I can still do.
Generally, I suggest merging this soon, so we can move forward with v1.

[zain-sohail]

Should we merge this?

[rettigl]

Should we merge this?

There is still something wrong in the tutorials. I will have a look. I will also give it another review then.

[rettigl]

Looks mostly good, apart from a few things still to fix. Let's resolve each point once addressed.

[rettigl]

@zain-sohail I fixed most of the issues. Please comment on the remaining ones, then we can merge here.