-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: parse parameters and units with DDParsers
#390
base: main
Are you sure you want to change the base?
Conversation
Evaluator
DDParsers
@sly2j the
correct? What about the others (see PR description above).. in particular Also, I don't think we necessarily need this, but do we have a file similar to DD4hepUnits.h for EDM4hep/eic? It could be convenient someday... |
9c95c69
to
f1898c0
Compare
So it is an issue that EDM4hep units don't match up with DD4hep. I was hoping we could adopt a single unit convention across the codebase. We are, obviously, tied to the the units in the EDM, so it would make sense to use its convention by default. While we do, currently, rely on DD4hepUnits.h, that is not a dependency. The only values in DD4hep units that we will see, will probably come from geometry (is TGeo unit-agnostic?). |
I think that's the general preference, but not sure if that's official yet.
Yeah, this |
Well, we'd probably need at least alternative constants for our system of units.
That is not a problem if we can leverage this: |
DDParsers
DDParsers
for more information, see https://pre-commit.ci
This PR already uses its wrapper, effectively calling setSystemOfUnits(/*EDM4hep system*/); You make a good point, we can leverage this for anything related to units, even unit conversion: EICrecon/src/utilities/eicrecon/param_parser_test.cc Lines 65 to 69 in 2f6395e
It could use some sugar, but this demonstrates that
|
I suggest we use something like eic/EDM4eic#35 |
Good idea! We can certainly then just use these here to set the parser's scales: EICrecon/src/utilities/eicrecon/parser.cc Lines 18 to 29 in 2f6395e
something like m_eval = std::make_unique<dd4hep::tools::Evaluator::Object>(
edm4eic::unit::m
// ...
) |
We could then consider adding some converter functionality, e.g. something to do number_in_edm_units = number_from_DD4hep * edm4hep::unit::mm / dd4hep::mm; Here's some pseudocode to sketch the general idea
/* dd4hep::Evaluator objects: contains dictionary for a system of units
* - this is what DD4hep uses to parse the compact file strings
* - they can be initialized with any custom unit system: you just need to say what is "1"
* for each unit type (see DD4hep source code for examples)
* - we may be able to use something simpler; there is a version from CLHEP too
*/
enum unit_systems { kDD, kEDM, nSys };
std::shared_ptr<dd4hep::tools::Evaluator::Object> eval[nSys];
eval[kDD] = std::make_shared<dd4hep::tools::Evaluator::Object>(/* DD4hep units specification */);
eval[kEDM] = std::make_shared<dd4hep::tools::Evaluator::Object>(/* EDM4hep units specification */);
/* general converter of `val` from one unit system `from` to another `to`
* - calls `Evaluator::Object::eval` to do the math and the dictionary lookup of `units_expr`
* - `units_expr` can be one unit, or an expression of units, e.g., "eV*nm/s"
* - `from` and `to` are `enum unit_systems` numbers, to specify the unit systems
* - if set to `-1`, use "natural unit system", where lack of an SI prefix really means "1"
* (in contrast, EDM4hep's length unit is "mm", so "m" does not mean "1" in that system)
*/
double ConvertUnits(
double val,
std::string units_expr,
int from,
int to
)
{
auto conv = [&eval, &units_expr, &nSys] (int u) {
if(u==-1)
return 1.0;
else if(u>=0 && u<nSys) {
auto result = eval[u]->eval(units_expr);
if(result.status() == dd4hep::tools::Evaluator::OK)
return result.result();
}
m_log->error("error");
return val;
};
return val * conv(to) / conv(from);
}
/* specialized converters (sugar)
* - uses ConvertUnits to do the conversion DD4hep <-> EDM4hep
* - example usage:
*
* auto sensor_size = ConvertUnitsDD2EDM(
* DD4hep_service->get_number("sensor_size"), // pseudocode; has DD4hep units
* "mm" // the units
* );
* // result: `sensor_size` will be in EDM4hep units
* // NOTE: "mm" could be any length unit, e.g. "m", or "nm"; this is because
* // "edm4hep::mm" / dd4hep::mm == "edm4hep::m" / dd4hep::m == etc.
*
*/
double ConvertUnitsDD2EDM(double val, std::string units_expr) {
return ConvertUnits(val, units_expr, kDD, kEDM);
}
double ConvertUnitsEDM2DD(double val, std::string units_expr) {
return ConvertUnits(val, units_expr, kEDM, kDD);
}
/* resolve units (sugar)
* - by default, any number in an algorithm can be assumed to have units in some unit system
* - printing this number out may not be clear, since the result will be that
* number together with the power of 10 representing the SI prefix(es), or
* other factors such as PI
* - use these "resolvers" to get the raw number in "natural" units, where
* lack of an SI prefix really means "1"
* - example usage
*
* // get a sensor size, in EDM4hep units (same as above example)
* auto sensor_size = ConvertUnitsDD2EDM(
* DD4hep_service->get_number("sensor_size"), // pseudocode; has DD4hep units
* "mm" // the units
* );
* // print it in [nm]; we know the unit system is EDM, so use `ResolveUnitsEDM`
* m_log->trace("sensor size is {} nm", ResolveUnitsEDM(sensor_size, "nm"));
*
*/
double ResolveUnitsDD(double val, std::string units_expr) {
return ConvertUnits(val, units_expr, kDD, -1);
}
double ResolveUnitsEDM(double val, std::string units_expr) {
return ConvertUnits(val, units_expr, kEDM, -1);
} |
I was hoping we could somehow dodge the need for conversion. Perhaps, wrap it into the DD4hep service API? Or, perhaps, override the global TGeoSystemOfUnits plus fix (potential) DD4hep bugs? |
Avoiding (automating) conversions would be ideal. What about |
Briefly, what does this PR introduce?
Use DD4hep/DDParsers to parse configuration parameters. This permits the user to include units that are automatically converted to an internal system of units. This internal unit system can be any preferred system we agree on and does not have to be DD4hep's system; e.g., it could be configured to parse parameters to EDM4hep's standard units.
Source code changes:
parser.{h,cpp}
: new classParser
:Evaluator
to do the parsing and mathParser
class can be pulled out of EICrecon, should we decide to use it elsewhere; we could also replace DD4hepEvaluator
usage with that of CLHEP, if we don't want DD4hep dependence hereeicrecon_cli.cpp
: useParser
to parseeicrecon -c
user parametersParser
Parser
could be used to read its numbersparam_parser_test.cpp
src/tests/
Some usage examples (defaulting to DD4hep's unit system, 1=cm=GeV):
It can also handle lists, parsing each element (this example uses EDM4hep's unit system, 1=ns=GeV):
Example unit systems:
EDM4hep units:
1 = millimeter = GeV = nanosecond = radian
Geant4 units:
1 = millimeter = MeV = nanosecond = nanoampere (via e+ charge) = kelvin = mole = candela = radian = steradian
This was assumed by
reco_flags.py
prior to #365DD4hep units:
Our current
eic-shell
build of DD4hep uses the following system:1 = centimeter = GeV = second = nanoampere = kelvin = mole = candela = radian = steradian
NOTE: see #365 for more discussion
The parser was fully compatible with the legacy
reco_flags.py
, and its results have been cross checked. Consistency with previous versions of EICrecon was verified by comparingrun_eicrecon_reco_flags.py
output, with the-c
option used foreicrecon
. The only differences are minor, typically dropping trailing zeros (e.g.,"4.0"
becomes"4"
).What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
No
Does this PR change default behavior?
Yes, the specification of parameters and units is enhanced