eic / EICrecon

EIC Reconstruction - JANA based
https://eic.github.io/EICrecon
GNU Lesser General Public License v3.0
6 stars 29 forks source link

Inconsistent parameter naming 'scheme' #318

Open wdconinc opened 1 year ago

wdconinc commented 1 year ago

Environment: (where does this bug occur, have you tried other environments)

Steps to reproduce: (give a step by step account of how to trigger the bug)

  1. Look at the parameter listing at the end of any eicrecon run

Expected Result: (what do you expect when you execute the steps above)

Some consistency in parameter naming... preferably something that is automated and doesn't allow free-form strings...

Actual Result: (what do you get when you execute the steps above)

  1. Acts (a project that is either spelled with all caps or with capital A) has parameters with lower case, and includes both acts:InitLogLevel and acts_init:LogLevel:
    acts:InitLogLevel                                                   : info
    acts:LogLevel                                                       : info
    acts:MaterialMap                                                    : calibrations/materials-map.cbor
    acts_init:LogLevel                                                  : info
  2. Log levels for algorithms follow different grammar, as for example in the following two parameters:
    B0ECAL:B0ECalClusters:depthCorrection
    B0ECalClusters:LogLevel
  3. But also, EEMC:B0ECalClusters:depthCorrection indicates that the B0ECalClusters somehow ended up in EEMC, while e.g. B0EcalClusters is in B0ECAL per above.
  4. Digi:SmearedFarForwardParticles:LogLevel does not appear connected to any other algorithm with parameters that can be set.
  5. jana:plugin_path uses lowercase, but JANA:STATUS_FNAME uses uppercase.
  6. Reco:GeneratedParticles:LogLevel with capital R, but reco:ReconstructedParticlesWithAssoc:LogLevel with lowercase r.
  7. HCAL includes barrel, endcaps, and insert, but BEMC and EEMC are for barrel and endcaps separate.
DraTeots commented 1 year ago

Related discussion:

https://github.com/eic/EICrecon/discussions/68

FYI Parameter names are not case sensitive (which is good). But indeed, there are lots of parameters with underscore '_' implying snake_case naming and other parameters rely on CamelCase. This bugs me a lot but before this ticket I thought I was the only one concerning and thinking about it as of a problem ;)

DraTeots commented 1 year ago

Note also that #68 was not concluded and no resolving policy is added to e.g. contribution.md. Thus it should be first decided and only then this ticket will become actual.

wdconinc commented 1 year ago

To be honest, I think this should not be a human decision. Too much opportunity for mistakes. There should be automatic parameter generation for all parameters that are available, with names that are unique and follow a specified set of rules.

DraTeots commented 1 year ago

To some degree it is automated. So the human decision is the last part and description. This pretty much work like "properties". If there was a policy defined in #68 we could fully automate it.

DraTeots commented 1 year ago

If, @wdconinc, you was supportive and allowed me to do https://github.com/eic/EICrecon/issues/296 - this would fix B0ECAL:B0ECalClusters:depthCorrection -> B0ECalClusters:LogLevel

As I would automate this part too. But now I don't want to go into each factory and fix it by hand.

~acts:InitLogLevel and acts_init:LogLevel - is a bug.~ But it is there because #68 doesn't fully define scheme

DraTeots commented 1 year ago

Ok, there is no acts_init:LogLevel flag. There are two separate loggers: acts and acts_init, controlled by 2 flags: acts:LogLevel and acts:InitLogLevel. The logic behind it (from the documentation)

ACTS has two loggers controlled by two flags:

The reason for such level split is because it might be important to have a verbose init loggins, where geometry conversion is printed out, while there is no need for full print of reconstruction and wise versa.

But names of loggers (things that are written in [square brackets]) are acts and atcs_init