add PU=0 at the start of vectors (master) #21291
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
A new Pull Request was created by @capalmer85 for master. It involves the following packages: SimGeneral/MixingModule @cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
hi @capalmer85 @civanch - so how do you suggest to prevent these issues next time? |
|
+1 |
|
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 (and backports should be raised in the release meeting by the corresponding L2) |
|
@davidlange6 , potentially, a warning may be added in the constructor of the class. May it be an exception for me not clear. |
|
For me, I have to remember that indices start at 0. :-)
I'm not sure what else one can do given that there is no single macro to
produce these PU distributions.
…On Tue, Nov 14, 2017 at 5:53 AM Vladimir Ivantchenko < ***@***.***> wrote:
@davidlange6 <https://github.com/davidlange6> , potentially, a warning
may be added in the constructor of the class. May it be an exception for me
not clear.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21291 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AFKqhzuVwUcQYizBZ7WHxVBB-e_xiMeFks5s2XDogaJpZM4Qb1UA>
.
|
|
humans are involved - no surprise - but maybe there is or should be a macro that _tests_ the distributions?
… On Nov 14, 2017, at 2:12 PM, capalmer85 ***@***.***> wrote:
For me, I have to remember that indices start at 0. :-)
I'm not sure what else one can do given that there is no single macro to
produce these PU distributions.
On Tue, Nov 14, 2017 at 5:53 AM Vladimir Ivantchenko <
***@***.***> wrote:
> @davidlange6 <https://github.com/davidlange6> , potentially, a warning
> may be added in the constructor of the class. May it be an exception for me
> not clear.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#21291 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AFKqhzuVwUcQYizBZ7WHxVBB-e_xiMeFks5s2XDogaJpZM4Qb1UA>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
What test do you suggest though? That the first PU index is 0? I'm not
sure that will always be true for all scenarios.
On Tue, Nov 14, 2017 at 8:32 AM, David Lange <notifications@github.com>
wrote:
… humans are involved - no surprise - but maybe there is or should be a
macro that _tests_ the distributions?
> On Nov 14, 2017, at 2:12 PM, capalmer85 ***@***.***>
wrote:
>
> For me, I have to remember that indices start at 0. :-)
>
> I'm not sure what else one can do given that there is no single macro to
> produce these PU distributions.
>
> On Tue, Nov 14, 2017 at 5:53 AM Vladimir Ivantchenko <
> ***@***.***> wrote:
>
> > @davidlange6 <https://github.com/davidlange6> , potentially, a
warning
> > may be added in the constructor of the class. May it be an exception
for me
> > not clear.
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub
> > <#21291 (comment)>,
or mute
> > the thread
> > <https://github.com/notifications/unsubscribe-
auth/AFKqhzuVwUcQYizBZ7WHxVBB-e_xiMeFks5s2XDogaJpZM4Qb1UA>
> > .
> >
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21291 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AFKqh2IYplgm4r_nbx0TDXNZEYTDApQaks5s2ZZcgaJpZM4Qb1UA>
.
|
|
hello @capalmer85 @davidlange6 a fair amount of test on the input histogram are actually carried out here: cmssw/Mixing/Base/src/BMixingModule.cc Line 99 in 09c3fce , and this is loaded any time the mixing takes place needing a target distribution.
That would be the easiest, imho. |
|
the bot can pick up any change in runTheMatrix. indeed, probably for the best that future pu distributions come with a corresponding runTheMatrix entry but that doesn't easily confirm the PU distribution itself. That could be more easily accomplished via a unit test.. maybe @civanch can discuss with the sim group and propose something? |
|
Yes, we can discuss this. For me not clear where check should be. Unit test is intended to test something stable but in this case after each new scenario we have to change unit test. This may not work. The extra sanity checks inside class constructor may be implemented and/or addition of an extra entry to the runMatrix may be needed. |
|
On Nov 15, 2017, at 11:04 AM, Vladimir Ivantchenko ***@***.***> wrote:
Yes, we can discuss this. For me not clear where check should be. Unit test is intended to test something stable but in this case after each new scenario we have to change unit test. This may not work. The extra sanity checks inside class constructor may be implemented and/or addition of an extra entry to the runMatrix may be needed.
or the unit test looks for all scenarios within the standard directory and does something useful with each one...
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
+1 |
Same as #21285 but in master