Get by token

[rfriese]

Hi,

a change in the Kappa data format was unavoidable: Accessing a InRun product is only available in the endRun method. EndRun is only called when already several times the lumi tree has been written. All other solutions than to introduce also a run tree would have been nasty fixes.

I would like to merge this branch into the dictchanges branch asap.

I'll prepare the necessary changes to Artus in the next days to use its output.

Cheers

[thomas-mueller]

master in Artus
tmpdictchanges in KITHiggs (dictchanges there might also work, but this is Olena's business)

[greyxray]

Dear Raphael,

does dictchanges branch on HTT not compiling? Do you scramb it together with dictchanges branch of Kappa?

[rfriese]

Hi,
I finally got it working, implementing also the functionally in KappaTools, Artus and KITHiggs (all repos on the getByToken branch).

I used the CMSSW8 test file, ran it through Kappa and the Run2SMAnalysis. The output is, regarding to compareRootFiles.py, identical.

To prevent further time-consuming merge conflicts I urge a bit and set the timeline for vetos to tomorrow at 12:00. Please let me know if you have any obligations.

Before merging to the master then I'll provide the manual testing procedure I already proposed, but this I will do on the dictchanges branch.

Cheers,
Raphael

[thomas-mueller]

Hallo Raphael,

I fear I have to redo the skim, which I did over the weekend and for which I already committed filelists on the tmpdictchanges branch in the kithiggs repository, in order to be able to continue usign this branch. In addition to testing the changes here I cannot do that until tomorrow noon.

Regards,
Thomas

[thomas-mueller]

I think another minimum requirement would be the test of CMSSW 7-9 (via travis).

[rfriese]

Hi Thomas,
the travis tests are all successful:
https://github.com/KappaAnalysis/Kappa/commits/getByToken

The merge in the KITHiggs and Artus Repositories can wait. In KITHiggs only one line changed:
https://github.com/cms-analysis/HiggsAnalysis-KITHiggsToTauTau/compare/tmpdictchanges...getByToken?expand=1
In Artus it's a bit more, but also nothing that'll cause conflicts:
https://github.com/artus-analysis/Artus/compare/getByToken?expand=1

So take your time testing your skim and the changes in Kappa. I just wanted to make sure, as you requested, that there's a full setup from Kappa to Artus output n-tuple. From the Kappa side I don't see the need to wait. The basic tests succeeded and it's just a merge to another development branch, nothing that will be used for production.

Cheers

[thomas-mueller]

You take the travis configuration from the dictchanges branch without modifications. This is why only this branch is checked out and tested.

[rfriese]

Thank you for pointing that out. It won't happen again:
72d071b

With the fixed config and locally it also runs fine.
https://travis-ci.org/KappaAnalysis/Kappa/builds/259770858?utm_source=github_status&utm_medium=notification

[pistone87]

Hi Raphael,

one question: what is the time scale for having Artus ready to run with the new skimming of Kappa?

[rfriese]

Hi Claudia,

the timescale is yesterday.

Use the getByToken Branches in KITHiggs, KappaTools and the Artus repository.

Cheers

[pistone87]

Ok, I will try that, thanks.

Cheers

[greyxray]

Hi Raphael,

I just merged the getByToken to dictchanges in Htt, since I am the only one who works on it now - my code worked with no issues and nothing changed.

Thank you,
Cheers

[thomas-mueller]

Hallo Raphael,

sorry, I was busy with other work the last few hours since the PR was announced. Now I checked it and see problems when running with increased verbosity (2) using this testing command:

----- Begin Fatal Exception 01-Aug-2017 19:30:43 CEST-----------------------
An exception of category 'LogicError' occurred while
[0] Processing run: 1 luminosityBlock: 34249
[1] Calling global beginLuminosityBlock for module KTuple/'kappaTuple'
Exception Message:> ::getByToken: An attempt was made to read a Run product before endRun() was called.
The index of the token was 7.
----- End Fatal Exception -------------------------------------------------

Olena confirms this. I do not get this problem before the merge of this PR.

Is it allowed to ask why we need to merge major changes under this pressure; whether we can urge you to provide a bugfix on the same time scale?

Regards,
Thomas

[rfriese]

Hi Thomas,

I just ran the command on the dictchanges branch and do not see the error. Did you recompile Kappa, maybe even with a scram b clean before? This error is exactly the reason the getByToken Branch required the extension of the Trees saved to the Kappa output. At the moment I can't see what should be the difference between the SUSY160GeV sample and the DY sample.

The PR was already there for 6 days. It passed all tests that you requested, and it was only a PR to the dictchanges branche where one usually not even has to do PRs. I got more and more busy solving merge conflicts and directly adjusting commits that just have been made. The testing setup is there and if problems arise I'm taking care of them. Same goes for the problem you posted above, just provide me with a reproducible example and I'll fix it.

Cheers

[thomas-mueller]

The problem occurs only with increased verbosity and remains after scram b clean + scram b.

[rfriese]

Thank you for the clarification. So it was also there in the standard test case and not something special to the wiki command.

it's fixed: ca1e899

[thomas-mueller]

Hallo Raphael,

I see, that event.m_genRunInfo is not a nullptr but all members are empty or have default values. For me it looks like the branch is correctly set but entries from the Runs tree never read (by the TTree::GetEntry function). Did you observe data being read from this tree during your tests and I am just missing something? (I can confirm that I am on the right branches in all four repositories, because otherwise the code would not compile.)

Regards,
Thomas

[thomas-mueller]

Hallo Raphael,

I see, that the KGenRunInfo struct does not contain a member for the run number as the KLumiInfo struct contains the run and lumi number. These numbers are crucial for the matching algorithm, which decides (on KappaTools level), which entry from the Runs and Lumi tree to read together with a given entry from the Events tree. The non-existence of the run number here lets me expect, that this matching algorithm is also not implemented yet and therefore the Runs cannot be used so far on analysis level (despite all promised tests).

We (everybody who relys on the dictchanges branch) are forced to use these trees due to you rushed and self-accepted merge of this PR. We had samples to use in Artus before this merge (as already written), but we cannot use them any longer because of additional dictchanges. The weekend is a perfect chance for a new skim (that we are forced to do due to this merge), but we need to check a test output of Kappa on Artus level first. You can imagine, that a fix of this issue is highly appreciated before the weekend (=today).

Regards,
Thomas

[rfriese]

did you notice that that KLumiInfo (collection name lumiInfo) is still saved?

It makes not sense at all to fill the lumi number in the Run tree...

[thomas-mueller]

Therefore I wrote:

I see, that the KGenRunInfo struct does not contain a member for the run number ...

I forgot to mention, that the Runs tree must also be read in for data and the mechanism to match the entries to the entries of the Events tree must work both for MC and data.

[thomas-mueller]

Dear Raphael, all

I see, that there is no reaction (appart from an assertion in KappaTools) so far regarding what I wrote last. I understand, that the idea is still not finalised and therefore reverted the merge of this PR in order to give it more time for more detailed discussions and checks.

I understand, that we are somehow forced by updates in CMSSW to write out some quantities in the endRun function, which is called once per CMS data taking run, which is usually identified by a run number. This change does in general not only apply to MC samples but to all samples, although at the moment there is more motivation to get it working for MC. The required changes touch the core of Kappa, KappaTools and also in the analysis code. In order to guarantee a well understood and safe transition from the well checked/maintained core to the new version, I vote for a long term solution, that supports this feature (writing out info in endRun functions) both for MC and data in a similar way we have it already for info at lumi section level.

In this sense, we should not restrict the code to run only on files, that contain only events from a single run but we should implement the same mechanism we already have for lumi sections and the Lumis tree in Kappa files also for runs and the Runs tree in Kappa files. In order to do the matching of entries in the Events tree to the Runs tree, we need the run number in both tree and we are missing it currently in the Runs tree for both data and MC. Adding this later (when we need this for data as well) would mean again a dataformat change but more importantly yet another big change in the core of the code which requires extensive tests. Btw. theses tests are basically impossible to automatise. You should check your implementation with dedicated print statements in the code!

What do you and others think?

Regards,
Thomas