Temporary fix of TQAF unit tests after TauID update #21186
Conversation
Customize to the unit tests that fail due to old input samples
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
A new Pull Request was created by @roger-wolf (Roger Wolf) for master. It involves the following packages: TopQuarkAnalysis/Examples @cmsbuild, @monttj 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 There are some workflows for which there are errors in the baseline: Comparison Summary:
|
|
Sorry, I had deleted this branch by mistake |
|
@roger-wolf |
|
@slava77 : I agree with replacing the relval files everywhere needed, so that we can get rid of the warning messages and take all discriminators into account also in the unit tests On the other hand, since we have defined the safety switch in the configuration, I would be in favor of still merging this PR as such anyhow, so that next time that the tau group wants to add or modify one of those discriminators we won't have to bother about those errors in the logs any more. In any case, this patch applies only to the UnitTests, while all normal workflows will keep crashing if the list of discriminators in the configuration does not correspond to those actually in the data file, which is the behavior we want for them. |
I disagree with a permissive solution. This way we open up to undetectable problems. |
|
Hi Slava,
could you clarify just for my understanding: you mean it would still be
useful to rewrite the configurable such that a list of discriminators
could be explicitly passed on that would be skipped in the PAT embedding
step? I think I might have some time that I can spend on TauPOG work of
this kind during the next days. So I can make another commit to this
branch. But I'd like to be clear what you would want us to do not to
waste to much of that time for solutions, that might never make it into
official-cmssw.
Cheers,
Roger
…On 22.11.2017 17:13, Slava Krutelyov wrote:
so that next time that the tau group wants to add or modify one of
those discriminators we won't have to bother about those errors in
the logs any more.
I disagree with a permissive solution. This way we open up to
undetectable problems.
From the beginning I was asking for this to be an exceptional setting,
better yet made specifying the list of IDs that can be skipped.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21186 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEwCUfpUYBKSESazfjcAM7X1FQGOPdqFks5s5EhBgaJpZM4QTdei>.
|
|
On 11/22/17 8:20 AM, Roger Wolf wrote:
Hi Slava,
could you clarify just for my understanding: you mean it would still be
useful to rewrite the configurable such that a list of discriminators
could be explicitly passed on that would be skipped in the PAT embedding
step? I think I might have some time that I can spend on TauPOG work of
this kind during the next days. So I can make another commit to this
branch. But I'd like to be clear what you would want us to do not to
waste to much of that time for solutions, that might never make it into
official-cmssw.
Hi Roger,
I'm somewhat OK with the current inclusive permissive solution,
but only as long as it stands as temporary.
It would be better to change the catch-all flag to a vstring,
which is empty by default and when filled will contain a list of taggers
that are "OK to ignore".
This is not an urgent improvement.
I think that now it is more important to update the inputs in all
relevant unit tests
to those with all needed taggers included in a replacement or update of
this PR.
…
Cheers,
Roger
On 22.11.2017 17:13, Slava Krutelyov wrote:
>
> so that next time that the tau group wants to add or modify one of
> those discriminators we won't have to bother about those errors in
> the logs any more.
>
> I disagree with a permissive solution. This way we open up to
> undetectable problems.
> From the beginning I was asking for this to be an exceptional setting,
> better yet made specifying the list of IDs that can be skipped.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#21186 (comment)>,
> or mute the thread
>
<https://github.com/notifications/unsubscribe-auth/AEwCUfpUYBKSESazfjcAM7X1FQGOPdqFks5s5EhBgaJpZM4QTdei>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21186 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbnW1363pdcogaqqkAybKZuiGnkeHks5s5Em_gaJpZM4QTdei>.
|
|
This is how I understood it. I'll try to have a look into this still
this week.
Thanx!
…On 22.11.2017 17:31, Slava Krutelyov wrote:
On 11/22/17 8:20 AM, Roger Wolf wrote:
> Hi Slava,
>
> could you clarify just for my understanding: you mean it would still be
> useful to rewrite the configurable such that a list of discriminators
> could be explicitly passed on that would be skipped in the PAT embedding
> step? I think I might have some time that I can spend on TauPOG work of
> this kind during the next days. So I can make another commit to this
> branch. But I'd like to be clear what you would want us to do not to
> waste to much of that time for solutions, that might never make it into
> official-cmssw.
Hi Roger,
I'm somewhat OK with the current inclusive permissive solution,
but only as long as it stands as temporary.
It would be better to change the catch-all flag to a vstring,
which is empty by default and when filled will contain a list of taggers
that are "OK to ignore".
This is not an urgent improvement.
I think that now it is more important to update the inputs in all
relevant unit tests
to those with all needed taggers included in a replacement or update of
this PR.
>
> Cheers,
> Roger
>
>
> On 22.11.2017 17:13, Slava Krutelyov wrote:
>>
>> so that next time that the tau group wants to add or modify one of
>> those discriminators we won't have to bother about those errors in
>> the logs any more.
>>
>> I disagree with a permissive solution. This way we open up to
>> undetectable problems.
>> From the beginning I was asking for this to be an exceptional setting,
>> better yet made specifying the list of IDs that can be skipped.
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <#21186 (comment)>,
>> or mute the thread
>>
>
<https://github.com/notifications/unsubscribe-auth/AEwCUfpUYBKSESazfjcAM7X1FQGOPdqFks5s5EhBgaJpZM4QTdei>.
>>
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#21186 (comment)>, or
> mute the thread
>
<https://github.com/notifications/unsubscribe-auth/AEdcbnW1363pdcogaqqkAybKZuiGnkeHks5s5Em_gaJpZM4QTdei>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21186 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEwCUSWjOvYx7rmXAmuXk1KgH-zX8-_pks5s5ExKgaJpZM4QTdei>.
|
|
merge i got far enough into my proposed solution to understand that this same set of fixes is already in a bunch of other cffs - so no real extra work is created by merging this |
|
Dear Slava, all,
On Wed, Nov 22, 2017 at 5:31 PM, Slava Krutelyov <notifications@github.com>
wrote:
On 11/22/17 8:20 AM, Roger Wolf wrote:
> Hi Slava,
>
> could you clarify just for my understanding: you mean it would still be
> useful to rewrite the configurable such that a list of discriminators
> could be explicitly passed on that would be skipped in the PAT embedding
> step? I think I might have some time that I can spend on TauPOG work of
> this kind during the next days. So I can make another commit to this
> branch. But I'd like to be clear what you would want us to do not to
> waste to much of that time for solutions, that might never make it into
> official-cmssw.
Hi Roger,
I'm somewhat OK with the current inclusive permissive solution,
but only as long as it stands as temporary.
It would be better to change the catch-all flag to a vstring,
which is empty by default and when filled will contain a list of taggers
that are "OK to ignore".
This is not an urgent improvement.
my two cents: personally I do not like the solution proposed by Slava with
an explicit list of taggers to ignore. I agree it is (hyper)correct, but
for me as a user who is potentially interested in running on top of old AOD
samples to produce MiniAOD ones, or a developer who potentially wants to
make some units tests not failing due to old inputs, the proposed solution
effectively does not differ from an explicit removal of items from the PSet
defining the list of taggers (not a big deal with python). The main
advantage of the current inclusive solution is the fact that it is
inclusive :)
Best,
Michał
|
Dear all,
this is a subsequent fix of unit tests in the TopQuarkAnalysis system as pointed out by Chris Jones (*). This is a minimal fix to the PR #21022 and should close this development step for the time being. We will envisage a better solution using eras in the midterm. All changes are on the level of final cfg files in test directories. Upon an independent test I confirm that tauID-related problems are indeed fixed.
Cheers,
Roger
(*)
#21022 (comment)