-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Adding HGCAL SuperCluster Regressions #32901
Merged
cmsbuild
merged 6 commits into
cms-sw:master
from
Sam-Harper:HGCALRegDevBranch_1130pre3
Feb 24, 2021
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
811d7e4
initial support for HGCAL in regression
Sam-Harper a92de71
setting phase2 GT
Sam-Harper 42dac06
fixing phase2 reco config
Sam-Harper affd483
bug fixes for HLT reg
Sam-Harper 36e6d29
code review comments
Sam-Harper c8f2356
removing dictionary and setting final GT
Sam-Harper File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
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
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.
from the use cases I don't see why default values are needed
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.
This is the result of two styles clashing. I prefer an initialise at construction approach while SCEnergyCorrectorSemiPara is a construct then initialise paradigm.
RegParam contains at EgammaBDTOutputTransformer which is has no way of altering its parameters after construction. However the RegParam objects only get their parameters latter. Therefore some defaults must be sent. I could just hard code the defaults into the constructor but to me it makes very little difference and in this way I can eventually adapt the class to my prefered paradigm.
Note this is the entire reason EgammaBTDOutputTransformer lost the constness of its two member variables, I needed the move constructor active and that was preferable to writing a custom one.
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 perhaps missed the line(s) in code where RegParam is used without defaults. Please clarify.
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.
sorry, I meant with defaults
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.
line 39, the default constructor of SCEnergyCorrectorSemiParm.
So I could get rid of that. But the problem is now we have to declare our transitions for ESProducts which I achieve through templates. But I dont want to template the class, just the method which calls it.
Now there are other solutions but are a few default parameters really so bad? I'm not seeing the problem.