JeffersonLab / hps-mc

HPS MC toolkit
1 stars 6 forks source link

Clarify usage of jinja function #381

Closed JeremyMcCormick closed 1 year ago

JeremyMcCormick commented 1 year ago

In ./python/hpsmc/job_template.py the function runnumber added into Jinja opens an LCIO file to read the first event and get the run number.

A couple comments on this:

1) Opening LCIO files when running the template engine seems somewhat out of scope from what was initially intended with this functionality. The intention was that the template processing only does light string manipulation while expanding the JSON job into a job store. It probably should not be opening LCIO data files to extract information from them.

2) This function will only work for LCIO files and not EVIO or ROOT, but it is named generically. If kept, I believe it should be called something like lcio_runnumber to make this clear.

3) It should be possible to achieve the same thing and extract the run number using string processing based on file-naming conventions. This would make opening the LCIO files unnecessary. (I believe we have an existing example of this?)

tomeichlersmith commented 1 year ago

I'm happy to rename the runnumber to lcio_dumpevent_runnumber to clarify its usage. I merely implemented this for a MC case (below) where the run number was not in the full path, but again, renaming it to show that it isn't very general (and opens the file) is a good idea.

eichl008@sdf-login02:~/hps/alignment/2019-fee$ head -1 2019-fee-mc-ideal-conditions.list 
/sdf/group/hps/mc/4pt55GeV/fee/idealCond/fee_recon_20um120nA_100.slcio
eichl008@sdf-login02:~/hps/alignment/2019-fee$ jq .[0] accum_jobs.json 
{
  "detector": "L3TAxial_tu_50um_iter0",
  "run_number": 1194550,
  "steering": "kf",
  "input_files": {
    "/sdf/group/hps/mc/4pt55GeV/fee/idealCond/fee_recon_20um120nA_100.slcio": "events.slcio"
  },
  "output_files": {
    "events_kf_mille.bin": "fee_recon_20um120nA_100_mille.bin",
    "events_kf_mille_gblplots.root": "fee_recon_20um120nA_100_gblplots.root",
    "mille_constraint.txt": "fee_recon_20um120nA_100_mille_constraint.txt"
  },
  "output_dir": "/sdf/group/hps/users/eichl008/hps/alignment/2019-fee/output/kf/L3TAxial_tu_50um_iter0",
  "steering_files": {
    "kf": "/sdf/group/hps/users/eichl008/hps/mc/examples/alignment/tracking/tracking_kf_alignment.lcsim",
    "st": "/sdf/group/hps/users/eichl008/hps/mc/examples/alignment/tracking/tracking_st_alignment.lcsim"
  },
  "job_id": 1
}
JeremyMcCormick commented 1 year ago

I'm happy to rename the runnumber to lcio_dumpevent_runnumber to clarify its usage.

I like this idea - makes it very clear what the macro does so that it doesn't seem like a general usage string parsing function.

I'm fine with keeping it in the jinja binding if this is done.