Skip to content

Draft: Introduce physlite flag for CP algorithm sequences

Nils Erik Krumnack requested to merge krumnack/athena:physlite_flag into master

The basic impetus here was to allow skipping the correction of the NOSYS systematic when running on PHYSLITE. The basic way I did that was to add a physlite flag into creating the sequence and to add a skipNominal flag to the algorithms that then allows to skip any action on the NOSYS systematic, except for creating shallow copies subsequent algorithms expect.

The three basic advantages I can see here:

  • adding a physlite flag for the sequences should make it easier to make further customizations for PHYSLITE
  • running with no systematics but nominal will still create all the shallow copies user code may expect if the algorithms had been run on PHYS, etc. but since it's not really doing a lot of work, it should be a rather lightweight thing to run on PHYSLITE.
  • it should also give a couple percent of performance improvement running with all systematics if the job isn't i/o limited. that's probably a less pressing issue at the moment.

I put in a basic implementation for the data handles, as well as for the muon sequence, just so we have a basis for discussion. Once we are convinced that we are happy with the general implementation I can then go ahead and add it for the other sequences. I did run the current muon sequence and it did finish successfully, but I didn't check the outputs.

My main open questions are:

  • On a technical level how should the actual skipping of the systematics work in the algorithm? If we were using SysListHandle::foreach I would just add the code in there (indeed I did, but no algorithm uses that). For now I just hard-coded a call to a helper function in each algorithm. An alternative would be to omit the nominal systematic from SysListHandle::systematicsVector and then provide a separate SysListHandle::preexecute function that does the copy (and that the algorithm has to call). I could also make it a side-effect for SysListHandle::systematicsVector, but that function having a side-effect would feel rather surprising to me, so I'd be strongly disinclined.
  • I currently added the skipNominal property to SysListHandle, meaning it is also available on algorithms for which it is not supported or doesn't even make sense. With the current implementation a reasonable alternative would be to add a skipNormal property to each algorithm that wants to support it. That would be somewhat more verbose, but not overly so. My bigger concern would be that if we want to change or extend this functionality at some point we have to go through all algorithms one-by-one and change them individually. Do people have an issue with having it on all CP algorithms?
  • I take it as a given that we would still want the algorithms to make copies for nominal even if they do nothing else. The main goal of the skipping was not to apply tools that wouldn't make any difference, creating copies would always make a difference, so I don't think we should skip it. However, do people have concerns in that regard?
  • Is a simple flag physlite good enough for what we want to do? Alternatively we could input a string with the derivation type in it. It would be more tedious for what we do right now, but it may give us more freedom later.

On the first point, I'm currently leaning more towards modifying systematicsVector and adding a preexecute (different from what is in the code right now), but I'd be curious what other people think before I do another further work.

Once we agree on what we want this to look like, I'd then go ahead make the agreed upon changes and add this for the other object types as well. There is probably more that could be done to reconfigure the sequences for PHYSLITE, but I'd rather leave that to the respective experts. At some point we should also add a set of PHYSLITE test files and then define tests on those as well, but that's probably also better left to another merge request.

A completely different approach would be not to build any infrastructure at all, and instead hand-code it on individual algorithms that need it. In which case I'd just close this MR and then open a separate MR in which I make sure all sequences have a physlite flag, and leave it to the CP experts to do with that flag whatever they consider necessary.

cc @tadej @jburr @lheinric @vimartin @schaarsc @zmarshal

Merge request reports