era 2017 updated with zs thresholds #21931
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-21931/2999 |
|
A new Pull Request was created by @amassiro (Andrea Massironi) for master. It involves the following packages: RecoParticleFlow/PFClusterProducer @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
Comparison Summary:
|
| @@ -19,22 +19,19 @@ | |||
| ) | |||
|
|
|||
| _particle_flow_zero_suppression_ECAL_2017 = cms.PSet( | |||
| thresholds = cms.vdouble(_pfZeroSuppressionThresholds_EB_2017 + _pfZeroSuppressionThresholds_EEminus_2017 + _pfZeroSuppressionThresholds_EEplus_2017 | |||
| thresholds = cms.vdouble(pfZeroSuppressionThresholds_EB + pfZeroSuppressionThresholds_EEminus + pfZeroSuppressionThresholds_EEplus | |||
There was a problem hiding this comment.
After a more careful look, I'd like to propose to move up the change to the arrays in the beginning of the file.
So, please leave this line unchanged and change the arrays above to be
#These are expected to be adjusted soon, while the thresholds for older setups will remain unchanged.
_pfZeroSuppressionThresholds_EB_2017 = pfZeroSuppressionThresholds_EB
_pfZeroSuppressionThresholds_EEminus_2017 = pfZeroSuppressionThresholds_EEminus
This implies that my current understanding is correct that the adjustments will apply only to 2017+ data periods while the range up to 2016 will remain unchanged.
If this is correct, when we need to make a change, that change will affect only the 2017 arrays above without a requirement to recode this line as well.
There was a problem hiding this comment.
For sake of a re-reco of all run2 collected data, also 2016 ones should have these thresholds included.
If I understand correctly how these modifiers are handled, with the current configuration if we run on 2016 we have:
particle_flow_zero_suppression_ECAL
while if we run on 2017/2018 the one to be used is:
_particle_flow_zero_suppression_ECAL_2017
Is this correct?
In the future we may need to put different thresholds depending on the year (definitely moving to Tags in DB would help a lot, but the exact values will have to be studied by POGs before deciding on the best values).
There was a problem hiding this comment.
For sake of a re-reco of all run2 collected data, also 2016 ones should have these thresholds included.
do you mean the future to-be-tuned thresholds?
How deep does this "for sake of re-reco" argument apply? If common thresholds are expected, they have to be implemented as common explicitly with a single common setting instead of copy-pasting.
Imagine we need to rereco Run-1 data. Would the updated thresholds apply there as well?
If so, then all the modifiers should be removed and we should just use the only thresholds in place in the arrays pfZeroSuppressionThresholds_EB, pfZeroSuppressionThresholds_EEminus, and pfZeroSuppressionThresholds_EEplus.
This file can then be cleaned up by removing everything after line 6
There was a problem hiding this comment.
Ok, now I see your point.
New commit with the suggested changes just done
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-21931/3020 |
|
On 1/24/18 9:20 AM, Andrea Massironi wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In
RecoParticleFlow/PFClusterProducer/python/particleFlowZeroSuppressionECAL_cff.py
<#21931 (comment)>:
> @@ -19,22 +19,19 @@
)
_particle_flow_zero_suppression_ECAL_2017 = cms.PSet(
- thresholds = cms.vdouble(_pfZeroSuppressionThresholds_EB_2017 + _pfZeroSuppressionThresholds_EEminus_2017 + _pfZeroSuppressionThresholds_EEplus_2017
+ thresholds = cms.vdouble(pfZeroSuppressionThresholds_EB + pfZeroSuppressionThresholds_EEminus + pfZeroSuppressionThresholds_EEplus
Ok, now I see your point.
New commit with the suggested changes just done
I'm still not sure that we are there yet.
The latest changes are in agreement with what I proposed assuming that
the plan is to keep the thresholds separate for the setup up to 2016
and then another one for 2017 and later.
The last comments from you before the latest commits implied that 2016
should have the same thresholds as in 2017.
I think that this is fine as well, but if that is the desired design, it
should be implemented that way.
In this case the change can be "_2017" -> "_Run2" in the arrays
and the modifier should be changed from run2_ECAL_2017 to run2_common.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21931 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbmazZtgHxq89_vbq52itIZmUdDtIks5tN2ZqgaJpZM4RqMr4>.
|
|
The best, from ECAL physics perspective, will be to have different settings for each year (2015, 2016, 2017, 2018) since the noise is evolving, or even more fine granularity, depending on what may get this year in terms of luminosity. Since, though, the changes of these settings can have non-negligible effects on physics performance, it has been decided so far not to touch them, but to keep them as they were in the past (Run 1). The final decision about re-tuning these thresholds, though motivated by (effective) noise increase in ECAL, will have to be agreed between POGs and PAGs, to limit the objects degradation and to improve performance. Studies are ongoing, and they have not reached a conclusion. The most important thing right now is to have for 2018 (MC and Data) the settings as they are set now (80 MeV and 300 MeV). Please, let us know what do you think the best implementation of this configuration would be, and would fit better the needs of central production, conformity among subdetectors, cleanliness. We don't have a strong opinion on this, apart from the fact that the thresholds are set correctly. |
|
On 1/24/18 11:29 AM, Andrea Massironi wrote:
The best, from ECAL physics perspective, will be to have different
settings for each year (2015, 2016, 2017, 2018) since the noise is
evolving, or even more fine granularity, depending on what may get this
year in terms of luminosity.
Since, though, the changes of these settings can have non-negligible
effects on physics performance, it has been decided so far not to touch
them, but to keep them as they were in the past (Run 1). The final
decision about re-tuning these thresholds, though motivated by
(effective) noise increase in ECAL, will have to be agreed between POGs
and PAGs, to limit the objects degradation and to improve performance.
Studies are ongoing, and they have not reached a conclusion.
The most important thing right now is to have for 2018 (MC and Data) the
settings as they are set now (80 MeV and 300 MeV).
Please, let us know what do you think the best implementation of this
configuration would be, and would fit better the needs of central
production, conformity among subdetectors, cleanliness. We don't have a
strong opinion on this, apart from the fact that the thresholds are set
correctly.
Settings varying by year and even more granular require a GT solution.
Your last comments have somewhat reverted the earlier statement that
2016 should have the same thresholds as 2017/2018.
I think it may be a lesser evil to stay with the current change (latest
commits).
- we were somewhat happy with the old thresholds for 2016 and could keep
them for now
- 2017 is not that different from 2018 and until a GT solution is
delivered it's ~OK to have the two options handled with the era (which
should not do mods that are supposed to be done with GT).
- we need a hook for studies of the thresholds that are not the same old
simple/fixed as in 2016
I'll leave the sign off until after the RECO meeting on Friday.
I hope this is OK.
…
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#21931 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbk6YpVgwMepnzg35cL5bHr6Fx_zqks5tN4R-gaJpZM4RqMr4>.
|
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
Comparison Summary:
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
From a physics point of view this should be the same as the current configuration, but it should be "cleaner", with less commented lines, and with the possibility to put later the best thresholds for both 2017 and 2018.
@crovelli @argiro