Skip to content
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

Refactor for maintainability and consistency #59

Merged
merged 6 commits into from
May 4, 2018

Conversation

kpedro88
Copy link

@kpedro88 kpedro88 commented May 4, 2018

The Python programming language supports a feature called "variables". Using this feature, the program can store the result of a computation and reuse it, rather than repeating the computation every time it is needed. In this PR, I have employed this feature for the string concatenations used to create module names. This makes it less likely for future developers to forget which collection to use, or to apply the postfix, etc.

In the process, I found a few places where the postfix was not applied correctly. These have been fixed. I also added an option to print all the available module names in order (stored in a dictionary) so the user can see what is available (more easily than doing a full edmConfigDump).

I tested this PR by creating a test config that exercised all the available options, dumping it, and comparing with a clean version. The only differences appear because of the aforementioned postfix fixes.

I noticed a few potential inconsistencies:

  1. In some places, addJetCollection is called with the postFix provided as a separate parameter. In other places, the postFix is included in the labelName parameter. I think the module names should be unique either way, but maybe there should be another look at this.
  2. The BoostedJetMerger for CMSTopTag is given patJets as the jetSrc. In other places, it is given the selectedPatJets.
  3. Some substructure quantities are named as jetALGO+'PF'+PUMethod+postFix while others are named as jetALGO+PUMethod+postFix (without the 'PF'). I'm not sure why this is.

Some other suggestions for future improvement (these are all fairly easy to implement, but I wanted to get agreement on this PR first):

  1. Remove QJets: I'm not even sure what that is, but it's been commented out for 3 years.
  2. Convert "warnings" for improper options to exceptions. It's better to stop the code from running when the user specifies something unsupported, rather than potentially letting them think it worked when it didn't (if they miss the warning message).
  3. Add overall verbosity control in case the user wants to suppress all printouts.
  4. Fix bTagDiscriminators = None behavior to avoid the default list (was proposed in Adding some tools and fixing bugs for next version #47 with other changes that proved controversial; the other changes should no longer be necessary, as the huge CPU usage I noticed in 80X was caused by prefetching inefficiencies that are no longer present in 94X).
  5. Fix root file creation with tasks (address Issue with Tasks implementation for 9X #51). Probably easiest just to add a switch that disables task association in case the user wants to do it themselves.

@alefisico alefisico merged commit 8c4d27b into cms-jet:jetToolbox_94X May 4, 2018
@kpedro88
Copy link
Author

kpedro88 commented May 4, 2018

@alefisico @rappoccio any comments on the various questions/suggestions?

@alefisico
Copy link
Member

Hi @kpedro88

  1. QJets was from the old jetToolbox (when it was inside CMSSW) so yeah I guess we can remove it
  2. and 3) Yeah why not :)
  3. and 5) true, it is something that we need to fix.

@kpedro88 kpedro88 mentioned this pull request May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants