From xml2 yaml

[siilieva]

Patient 0 to have YAML-based input parameter settings.

[siilieva]

Not absolutely sure about the formatting of entries having value&unit as in e.g.
max_angle:
required unit: rad
value: 1.
Such approach makes the file longer. On the other side it is clearer and less prone to errors compared to one liners e.g.
max_angle: {value: 1, unit: rad}

[olantwin]

Concerning the units, do we plan to use them in the code? For now, I think a comment would be sufficient.

One improvement I notice is that yaml can parse numbers etc., while with the XML we used to only parse them from the text.

There are python libraries for dealing with quantities with units, some even with good numpy support, but I think a huge amount of code would have to be adjusted, and they're probably not at all compatible with ROOT.

Superficially this looks all good, but I can only test the code in practice tomorrow.

[siilieva]

I just realized - we have shipUnits module. I can link the unit in code to the unit in the yml file so that we would have unit support too. I will try that tomorrow.

[ThomasRuf]

What is the difference between the current use case and the loading of the geometry/config parameters for the detector elements which we already have? All you need is a mapping of parameter name and value. Using python to make this dictionary allows also to use any arithmetic operation. The adding of comments in a python file is up to the developers. My personal opinion.

[olantwin]

I just realized - we have shipUnits module. I can link the unit in code to the unit in the yml file so that we would have unit support too. I will try that tomorrow.

In that case, maybe we should expect the string, parse it using shipUnits and throw an error, if the units isn't recognised?

[siilieva]

I just realized - we have shipUnits module. I can link the unit in code to the unit in the yml file so that we would have unit support too. I will try that tomorrow.

In that case, maybe we should expect the string, parse it using shipUnits and throw an error, if the units isn't recognised?

exactly

[siilieva]

What is the difference between the current use case and the loading of the geometry/config parameters for the detector elements which we already have? All you need is a mapping of parameter name and value. Using python to make this dictionary allows also to use any arithmetic operation. The adding of comments in a python file is up to the developers. My personal opinion.

The outline of the yaml file is indeed very clean, makes it a lot more user friendly than config.py or XML. Depending on the structure of the yml file, its contents are automatically interpreted by python using the most suitable data structure (list/dictionary/else). So we lose nothing here. We gain easier reading in c++ than with python, without the need to have the Get/Set methods.
Comments in the yml are also eazy to make(just like in py code). My concern(now solved) was I don't want to use comments for units as these can be useful.

[olantwin]

@siilieva We should probably write a short helper function to parse a unit and reuse it instead of rewriting the check everywhere.
Apart from that, the unit support looks great!

[antonioiuliano2]

Thank you Simona, it indeed seems a very useful tool to provide parameters

[siilieva]

for now, I will add the helper function in the tracking task itself.
Potentially in the future, once we move to the other tools (analysis ones and utilities) we should migrate the unit checker function to the utilities tools.

[ThomasRuf]

What is the difference between the current use case and the loading of the geometry/config parameters for the detector elements which we already have? All you need is a mapping of parameter name and value. Using python to make this dictionary allows also to use any arithmetic operation. The adding of comments in a python file is up to the developers. My personal opinion.

The outline of the yaml file is indeed very clean, makes it a lot more user friendly than config.py or XML. Depending on the structure of the yml file, its contents are automatically interpreted by python using the most suitable data structure (list/dictionary/else). So we lose nothing here. We gain easier reading in c++ than with python, without the need to have the Get/Set methods. Comments in the yml are also eazy to make(just like in py code). My concern(now solved) was I don't want to use comments for units as these can be useful.

Get and set methods were all replaced long time ago by one method, no need to write a get/set method for each parameter when loading the geometry and instantiating the detector classes.

[ThomasRuf]

Just my personal opinion. Having the conversion to ROOT units on the input side, for example:
c.EmulsionDet.Xpos0,c.EmulsionDet.Ypos0,c.EmulsionDet.Zpos0 = 53.5u.mm,2889.2u.mm,172.0u.mm
looks to me much better than specifying in the input file in what unit the user should specify the input parameter, and then make a conversion on the other side. In the above example, it would not be problem if the user prefers cm.
c.EmulsionDet.Xpos0,c.EmulsionDet.Ypos0,c.EmulsionDet.Zpos0 = 5.35u.cm,288.92u.cm,17.20u.cm would give the same result.

[siilieva]

Just my personal opinion. Having the conversion to ROOT units on the input side, for example: c.EmulsionDet.Xpos0,c.EmulsionDet.Ypos0,c.EmulsionDet.Zpos0 = 53.5_u.mm,2889.2_u.mm,172.0_u.mm looks to me much better than specifying in the input file in what unit the user should specify the input parameter, and then make a conversion on the other side. In the above example, it would not be problem if the user prefers cm. c.EmulsionDet.Xpos0,c.EmulsionDet.Ypos0,c.EmulsionDet.Zpos0 = 5.35_u.cm,288.92_u.cm,17.20_u.cm would give the same result.

I have to see if py imports can be done in the yaml file, cause that unit control is indeed much more elegant.

[siilieva]

I could not figure out how to link the shipunit.py module to the yaml without making a copy of former, so I will drop that effort.

[siilieva]

What is the difference between the current use case and the loading of the geometry/config parameters for the detector elements which we already have? All you need is a mapping of parameter name and value. Using python to make this dictionary allows also to use any arithmetic operation. The adding of comments in a python file is up to the developers. My personal opinion.

The outline of the yaml file is indeed very clean, makes it a lot more user friendly than config.py or XML. Depending on the structure of the yml file, its contents are automatically interpreted by python using the most suitable data structure (list/dictionary/else). So we lose nothing here. We gain easier reading in c++ than with python, without the need to have the Get/Set methods. Comments in the yml are also eazy to make(just like in py code). My concern(now solved) was I don't want to use comments for units as these can be useful.

Get and set methods were all replaced long time ago by one method, no need to write a get/set method for each parameter when loading the geometry and instantiating the detector classes.

it is one function, but for each class, no? We have the SetConfPar / GetConfPar(I/F/S) for all detector classes.

[ThomasRuf]

yes. But if all analysis functions/tools inherit from the same base class, it is only one method.

[siilieva]

yes. But if all analysis functions/tools inherit from the same base class, it is only one method.

ok. Since we have separate hit classes I though of having separate analysis classes for scifi and mufi hits, and perhaps one common for event analysis combining all information available per event.

[ThomasRuf]

this sounds strange. What about analysis tools which require scifi and mufi hits?

[siilieva]

Tools requiring hits from different detectors would be 'event analysis tools'.
I suspect having only one tool class would have it explode in size with too many functions.

[ThomasRuf]

you want to make one class with many functions? Also strange. I would have made one tool for every use case. Not having one class which is supposed to do everything.

[olantwin]

Sorry that I never reviewed this. I still think this would be a nice improvement, and the implementation looks very straight-forward and clean.