Closed ralfschmieder closed 2 years ago
I would say most of the changes look reasonable, but a few things need to be changed in the PR:
I think it could also make sense to tag the last commit before your changes with a proper name to make sure we have a reference for the last version of the code that works on Artus ntuples in case we would introduce backward incompatibilities later on and ever need to process Artus ntuples.
Jumping in here (i am no expert with the newer framework), but would it make sence, to use different classes for CROWN and ARTUS ntuples, but still keep both functions, and then have something like backend=="CROWN
, which would default to ARTUS is nothing is set ? This would require no changes when running with ARTUS ntuples and for CROWN ntuples it would be adding a single additional keyboard. This way we would not have a more or less stale tag for ARTUS
If I have seen it correctly, your suggestion is more or less what is implemented in this PR so far. So there now is an additional class for crown datasets that might not even be necessary at the moment as it only differs in one member and argument in the constructor. The backend keyword argument might not be necessary as we currently have two different functions to create the dataset objects that are called by the configuration code for the histogram production. So it is merely a style choice if we would prefer to have a single function with backend
keyword or two separate functions. The first option might be a little sparser as some parts of the code are shared at the moment.
Concerning the tag, technically tags are not really stale by definition I would say as they simply mark a state of the repository at some point(as for example tagging a version/release). I have proposed the tag as I thought we might only keep the code based on artus ntuples around during the transition period and possible remove this stale part of the code in the future. Also I wanted to prevent our bad habit of opening branches for some changes and then leave them stale as we do not need them any more but I am open for discussion on not using a tag and continue the support for both backends.
included CROWN ntuple processing based on dev branch, which is used in the NMSSM analysis framework