Skip to content
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

MiniAOD part 3 (7.0.X version) #4008

Merged
merged 30 commits into from Jun 4, 2014
Merged

Conversation

gpetruc
Copy link
Contributor

@gpetruc gpetruc commented May 26, 2014

7.0.X version of #3946 and #3875, plus an extra bugfix to PUJetId on ak4 jets.

Tested on CMSSW_7_0_X_2014-05-26-0200 with

cmsDriver.py miniAOD_data-prod -s PAT -n 100 --data --filein /store/relval/CMSSW_7_0_0/SingleMu/RECO/GR_R_70_V1_RelVal_zMu2012D-v2/00000/0259E46E-F698-E311-8CFD-003048FF9AC6.root  --eventcontent MINIAOD --runUnscheduled --conditions GR_R_70_V1::All --no_exec
cmsDriver.py miniAOD-prod -s PAT -n 100 --mc --filein /store/relval/CMSSW_7_0_0/RelValTTbar_13/GEN-SIM-RECO/PU50ns_POSTLS170_V4-v2/00000/265B9219-FF98-E311-BF4A-02163E00EA95.root  --eventcontent MINIAODSIM --runUnscheduled --conditions auto:startup --no_exec
cmsDriver.py miniAOD-fsim-prod -s PAT -n 100 --mc --fast --filein /store/relval/CMSSW_7_0_0/RelValTTbar_13/GEN-SIM-DIGI-RECO/POSTLS170_V3_FastSim-v2/00000/109B6DF8-A798-E311-A88F-02163E00A0F0.root --eventcontent MINIAODSIM --runUnscheduled --conditions POSTLS170_V3::All --no_exec

@arizzi @bendavid

arizzi and others added 29 commits May 14, 2014 21:44
  * remove r9, sigmaIphiIphi from pat::Electron (in new releases it
    is already in reco::GsfElectron)
  * duplicate the 7.1.X interface of reco::GsfElectron from
    PR cms-sw#3710 for the non-ZS ones, so that the 70X code will
    be forward-compatible with 71X
…nd photons with consistently relinked clusters and conversions
… behaviour of keeping supercluster and seed cluster for all output electrons and photons
…loose matching for reduced conversion collection)
Tested:
1) cmsDriver.py miniAOD_data-prod -s PAT -n 100 --data --filein /store/relval/CMSSW_7_0_0/SingleMu/RECO/GR_R_70_V1_RelVal_zMu2012D-v2/00000/0259E46E-F698-E311-8CFD-003048FF9AC6.root --eventcontent MINIAOD --runUnscheduled --conditions GR_R_70_V1::All --no_exec
2) cmsDriver.py miniAOD-prod -s PAT -n 100 --mc --filein /store/relval/CMSSW_7_0_0/RelValTTbar_13/GEN-SIM-RECO/PU50ns_POSTLS170_V4-v2/00000/265B9219-FF98-E311-BF4A-02163E00EA95.root  --eventcontent MINIAODSIM --runUnscheduled --conditions auto:startup --no_exec

Conflicts:
	PhysicsTools/PatAlgos/python/slimming/miniAOD_tools.py
(--fast will replace some fastsim-incompatible MET filters with
 dummy boolean filters)
Tested successfully with: cmsDriver RelValTTbar_13-POSTLS170_V3_FastSim-v2 -s PAT -n 10000 --mc --fast --dasquery "file dataset=/RelValTTbar_13/CMSSW_7_0_0-POSTLS170_V3_FastSim-v2/GEN-SIM-DIGI-RECO" --eventcontent MINIAODSIM --runUnscheduled --conditions POSTLS170_V3::All --no_exec
the process.options so not to mix up the allowUntracked
…esn't get added to the jets used for MET systematics (fixes regression after bugfix to to the PAT UserData Merger)
@gpetruc
Copy link
Contributor Author

gpetruc commented Jun 3, 2014

Hi Slava,

Why are you trying to forward port to 7.2.X the 7.0.X code?
It makes no sense: the 7.2.X equivalent code for those functionality is
either already integrated in the part3 (merged may 21) or queued in part4.
The differences between the 7.0.X and 7.2.X codes are international,
motivated by the differences in event content, dataformats or functionality
between the two releases

 Giovanni

Il 3-giu-2014 05:27 "Slava Krutelyov" notifications@github.com ha scritto:

I tried to do the forward port of this back into 72X (to see what's going
to happen after it moves over there and to see what's new here compared to
72X).

The results are here:
slava77@cms-sw
:CMSSW_7_2_X...CMSSW_7_2_X/from_7_0_X_2014-06-03-1400/fwport4008
Things look fine for code changes for 70X.

Since the automated forward ports are not disabled, these are worth a
note:

Change in the ConfigBuilder should've probably already been in 72X
slava77@cms-sw:CMSSW_7_2_X...CMSSW_7_2_X/from_7_0_X_2014-06-03-1400/fwport4008#diff-05daab1ca94b26f7bab27d1187d95415R587
this is probably OK
Changes in the PAT lepton classes with full5x5 shower shape added should
be made disappear in 71X and 72X.

The commands to fw port are (starting from a 70X release)

git cms-merge-topic 4008
git fetch official-cmssw
git branch source
git reset --hard official-cmssw/CMSSW_7_2_X
git clean -fdx
git merge -X ours source


Reply to this email directly or view it on GitHub.

@slava77
Copy link
Contributor

slava77 commented Jun 3, 2014

Hi Giovanni,

the CMSSW integration includes automated forward ports from 70X to 71X and 72X and from 71X to 72X.
It's been like this since 71X was opened for developments.

@slava77
Copy link
Contributor

slava77 commented Jun 3, 2014

On a technical side, the commands are a pretty easy way to see that the backport was done appropriately, as it's easy to miss things in 1000+ line diffs

@bendavid
Copy link
Contributor

bendavid commented Jun 3, 2014

Hi Slava,
This makes no sense at this point for 70x -> 71x.

For 71x -> 72x it maybe still makes sense given that the releases have not diverged much.

For the MiniAOD pull requests this automatic forward porting makes even less sense as there are explicitly different pull requests being prepared for 70x and 71x which cover the same functionality...

@slava77
Copy link
Contributor

slava77 commented Jun 3, 2014

Hi Josh, you are welcome to comment on this today in the ORP.

@gpetruc
Copy link
Contributor Author

gpetruc commented Jun 3, 2014

It makes no sense at all to automatically forward port our 7.0.X
request to newer releases, so look at the diffs if you like but don't
merge
them anywhere.

Giovanni

On Tue, Jun 3, 2014 at 12:10 PM, Josh Bendavid notifications@github.com
wrote:

Hi Slava,
This makes no sense at this point for 70x -> 71x.

For 71x -> 72x it maybe still makes sense given that the releases have not
diverged much.

For the MiniAOD pull requests this automatic forward porting makes even
less sense as there are explicitly different pull requests being prepared
for 70x and 71x which cover the same functionality...


Reply to this email directly or view it on GitHub
#4008 (comment).

@slava77
Copy link
Contributor

slava77 commented Jun 3, 2014

@ktf
Maybe Giulio can comment if there's a way to take out one PR, from what I know there's no way to stop this for one PR, it (fw porting) is done at the level of the release branch.

@arizzi
Copy link
Contributor

arizzi commented Jun 3, 2014

hi slava, can you explain better what that is supposed to mean?
In particular I'd like to point out that
a) we mostly develop in 70X as CSA14 is our first goal
b) you guys asked to forward port and integrate first in 72X, then in 71X, then in 70X each bunch of requests
c) we did as in (b) and have separate PR for same feature-sets in the three releases.

Now, why should we have an automatic forward port too? just to check what's left out if anything? or are you thinking about merging those? I mean some stuff make sense only in 70X because, for example, reco changed in 71X (see ivf or ak4) and some stuff does not need to be re-run in 71X at miniAod making time (while is needed in 70X)

@slava77
Copy link
Contributor

slava77 commented Jun 3, 2014

Hi Andrea

The release integration system is set up such that there are automated forward ports from earlier 7X to later 7X branches.
All changes aiming for a backport should be tested in a development branch first.
A combination of the two should explain what's happening.

@gpetruc
Copy link
Contributor Author

gpetruc commented Jun 3, 2014

Ok, so, let's put it this way: from our point of view, we were asked to
provide some 7.0.X code for CSA14 and we have done it and put it in a PR
for 7.0.X
We leave it up to offline to decide what you want to do with it (forward
ports, backflips, cartwheels, A_{fb} measurements, ...), if what ends up in
7.2.X or 7.1.X is different from what we queued for those releases (and
thus wrong) it doesn't matter since nobody is going to use them on the
short timescale.
Just let us know whether you can integrate it in 7.0.X or whether we should
give up the idea of central production of miniAODs for CSA14.

On Tue, Jun 3, 2014 at 12:37 PM, Slava Krutelyov notifications@github.com
wrote:

Hi Andrea

The release integration system is set up such that there are automated
forward ports from earlier 7X to later 7X branches.
All changes aiming for a backport should be tested in a development branch
first.
A combination of the two should explain what's happening.


Reply to this email directly or view it on GitHub
#4008 (comment).

@arizzi
Copy link
Contributor

arizzi commented Jun 3, 2014

@slava77 how can that make sense for stuff such as PAT that depends on the actual RECO content???

@bendavid
Copy link
Contributor

bendavid commented Jun 3, 2014

I thought that for 70x->71x this was in place only at the very beginning of the release cycle and was turned off long ago. Agreed that we had better discuss it in the ORP today...

(Also for 71x->72x it MIGHT still make sense, but if all changes are anyways forced to be integrated in 72x first anyways, then there is clearly no point...)

@davidlange6
Copy link
Contributor

Hi Josh

On Jun 3, 2014, at 1:01 PM, Josh Bendavid notifications@github.com
wrote:

I thought that for 70x->71x this was in place only at the very beginning of the release cycle and was turned off long ago.

Just to clarify - this is not the case (and nor should it be given the frequency of fixes that go in and are not forward ported). The trouble of course comes that there is a bit of additional difficulty when pushing new developments into old releases…

Cheers-
David

Agreed that we had better discuss it in the ORP today...

(Also for 71x->72x it MIGHT still make sense, but if all changes are anyways forced to be integrated in 72x first anyways, then there is clearly no point...)


Reply to this email directly or view it on GitHub.

@slava77
Copy link
Contributor

slava77 commented Jun 3, 2014

+1

tested in #4008 002e442

No differences in DQM or validation (except for one case below), tested on top of CMSSW_7_0_X_2014-06-03-0200

@slava77
Copy link
Contributor

slava77 commented Jun 3, 2014

Here is a diff that showed up in tests of this PR.
After looking at it a bit, I think it's unrelated to this PR, likely some instability due to a bad value or smth.
There was a difference in one gamma 35 event (out of 2K, wflow 19.0): allConversions have 2 more conversions with new code in one event, all are arising from a mix of the same tracks.
If somebody cares to investigate, in CMSSW_7_0_X_2014-06-03-0200
runTheMatrix.py -l 19.0 --useInput all --command="-n 2000" >& run19.0_2k.log &

eo->Scan("recoConversions_allConversions__RECO.obj.theTrackPin_.Rho():en.recoConversions_allConversions__RECO.obj.theTrackPin_.Rho()", "recoConversions_allConversions__RECO.obj@.size()!=en.recoConversions_allConversions__RECO.obj@.size()")
*      126 *        0 * 12.694513 * 12.694513 *
*      126 *        1 * 1.6174942 * 1.5842647 *
*      126 *        2 *           * 12.694513 *
*      126 *        3 *           * 1.6174942 *
*      126 *        4 *           * 16.180294 *
*      126 *        5 *           * 1.5842647 *

@ktf ktf mentioned this pull request Jun 4, 2014
ktf added a commit that referenced this pull request Jun 4, 2014
@ktf
Copy link
Contributor

ktf commented Jun 4, 2014

Now that #4107 is merged we can safely merge this one, which will not have any side effects into 71X.

ktf added a commit that referenced this pull request Jun 4, 2014
@ktf ktf merged commit ff5b1e7 into cms-sw:CMSSW_7_0_X Jun 4, 2014
@StoyanStoynev StoyanStoynev mentioned this pull request Jun 19, 2014
@gpetruc gpetruc deleted the miniAOD-from704-part3 branch September 30, 2015 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants