-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[PF] Add fillDescription to RecoParticleFlow producers #45212
Merged
Merged
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
41f08d0
Add fillDescription to RecoParticleFlow producers
7b34707
Fixed issue and implemented comments
50d3e58
Remove exists and fix fill descriptions
2da72f0
Remove the unknown PF parameter settings
a848a6c
Implemented comments
c85415b
Remove optional condition on parameters
9fee0bb
Undo the renaming of gathering, seeding and reHitEnergyNorm double ve…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why adding all these new parameters here - when there is the fillDescriptions methods which take care of providing them if they are not in the py config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the values to be used different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because when I tested them, if there were not present in the menu but were expected in the fillDescription, it returned an error of missing parameter. So this checks if the parameter is present, and if it is not, then adds it with a default value to the HLT module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or are you referring to the recHitEnergyNorms , seedingThresholds and gatheringThresholds parameters?
If yes, it is because there were conflicts of two different types (double and std::vector<"double">) used with exactly the same parameter name (not allowed in fillDescription (got an error of duplicate name)), so I created two sets of parameters:
So the python macro also checks for the parameters which are type std::vector<"double"> with names (seedingThreshold , gatheringThreshold and recHitEnergyNorm) and effectively renames them to the new names (seedingThresholds , gatheringThresholds and recHitEnergyNorms)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then please add an appropriate default for the HLT in the
fillDescription
itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I think the issue is probably related to them been declared optional. I will try to change them from optional to normal parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see also #45212 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So when I replaced the exists (checking that a given label of a pset was present) with checking the emptiness of the given parameter sets, it prompted the situation where a parameter set would then be by default empty but could also have parameters, so I made the parameters of those psets optional. However the moment those psets are not empty anymore, then some of the parameters are read and it triggers the missing parameter error when they are not present (since they were declared as optional).
I will check how to deal with these cases (probably avoid checking that the pset is empty and instead use another criteria to replace the previous exists conditions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it in c85415b , such that now all the parameters I added in the fillDescription are no longer optional. Also fixed the customizeForCMSSW.py file such that now it only deals with the renaming of the double vector parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I managed to undo the renaming of the double vector parameters in 9fee0bb , so now I don't customize the menu anymore, and this change helped reduce the number of changed files by 10