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

Porting changes for beaudett: was pullID 120 #156

Closed
wants to merge 27 commits into from

Conversation

deguio
Copy link
Contributor

@deguio deguio commented Jul 21, 2013

including in the pull request 120 the last changes to the files:
Validation/RecoEgamma/python/egammaValidation_cff.py
Validation/RecoEgamma/plugins/PhotonPostprocessing.cc

which ware lost during the porting to GIT

@deguio
Copy link
Contributor Author

deguio commented Jul 21, 2013

@beaudett
I have rejected the pull request 120 and merged your changes with the updates to the files in [1] in the new pull request 156.

Since I am new to git, please have a look and cross check that everything is correct.

thanks,
F.

[1]
Validation/RecoEgamma/python/egammaValidation_cff.py
Validation/RecoEgamma/plugins/PhotonPostprocessing.cc

@cmsbuild
Copy link
Contributor

The following categories have been signed by @deguio: DQM

@cms-git-dqm

@beaudett
Copy link
Contributor

Hello,

I am taking a look right now. I am new to git as well and I'll do my best.

Thanks
Florian

On 21/7/13 6:07 PM, deguio wrote:

@beaudett https://github.com/beaudett
I have rejected the pull request 120 and merged your changes with the
updates to the files in [1] in the new pull request 156.

Since I am new to git, please have a look and cross check that
everything is correct.

thanks,
F.

[1]
Validation/RecoEgamma/python/egammaValidation_cff.py
Validation/RecoEgamma/plugins/PhotonPostprocessing.cc


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

@beaudett
Copy link
Contributor

Hello,
[adding Lindsey]

it runs fine, and it does what it is supposed to do.
Looking more carefully, I have however some doubts about a change in
RecoParticleFlow/PFProducer/src/PFEGammaAlgo.cc
that you copied from he:
deguio@a4eaf31

Lindsey, from the pull request page, it seems that you had anticipated
such possible problem. Should be go ahead this way ?

Florian

On 21/7/13 6:07 PM, deguio wrote:

@beaudett https://github.com/beaudett
I have rejected the pull request 120 and merged your changes with the
updates to the files in [1] in the new pull request 156.

Since I am new to git, please have a look and cross check that
everything is correct.

thanks,
F.

[1]
Validation/RecoEgamma/python/egammaValidation_cff.py
Validation/RecoEgamma/plugins/PhotonPostprocessing.cc


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

@ktf
Copy link
Contributor

ktf commented Aug 5, 2013

@deguio, have you pushed the stuff you reverted?

@ghost ghost assigned deguio Aug 6, 2013
@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 6, 2013

The following categories have been rejected by @vadler: Analysis

@cms-git-analysis

1 similar comment
@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 6, 2013

The following categories have been rejected by @vadler: Analysis

@cms-git-analysis

@vadler
Copy link

vadler commented Aug 6, 2013

I rejected it for the moment, since the changes at DataFormat level (renaming of data members) are not propagated to PAT and beyond. I will assist @beaudett to get this done, based on https://hypernews.cern.ch/HyperNews/CMS/get/swReleases/3922/1/1/1.html.
The MCMatcher issue in David's log file should be uncorrelated to that.

@vadler
Copy link

vadler commented Aug 20, 2013

Finally, things become clearer, thanx to Chris and Philippe. To summarise it here:

  • The MCMatcher issue is a consequence of the changes in this topic.
  • It seems to a ROOT bug.

Details can be found in https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3152.html ff.
I will stay with my rejection for the moment, since there is nothing, AT can do about it. If this topic should go in now, the ROOT ioread directives have to be removed and backward incompatibility accepted for the time being, until the ROOT fix becomes available.

@Dr15Jones
Copy link
Contributor

Here is the issue posted by Philippe Canal to the ROOT bug tracking system

https://sft.its.cern.ch/jira/browse/ROOT-5437

@beaudett
Copy link
Contributor

Hello,

I don't know the policy about the backward incompatibility in this
release. Given that it is the third attempt to get this change in, and
each time it is submitted it requires ~120 packages to be recompiles, I
hope that the (temporary) backward incompatibility is acceptable at this
stage of the release building.
Slava & Thomas, please let us know.

Cheers,
Florian

On 20/8/13 11:58 PM, Volker Adler wrote:

Finally, things become clearer, thanx to Chris and Philippe. To
summarise it here:

  • The MCMatcher issue /is/ a consequence of the changes in this topic.
  • It seems to a ROOT bug. Details can be found in
    https://hypernews.cern.ch/HyperNews/CMS/get/edmFramework/3152.html
    ff. I will stay with my rejection for the moment, since there is
    nothing, AT can do about it. If this topic should go in /now/, the
    ROOT |ioread| directives have to be removed and backward
    incompatibility accepted for the time being, until the ROOT fix
    becomes available.


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

@Dr15Jones
Copy link
Contributor

The problem has been fixed in ROOT

@ktf Giulio, can we get this ROOT patch in?
https://sft.its.cern.ch/jira/browse/ROOT-5437

@ktf
Copy link
Contributor

ktf commented Aug 26, 2013

@davidlt is looking into this right now.

@ktf
Copy link
Contributor

ktf commented Aug 29, 2013

@davidlt ping?

@davidlt
Copy link
Contributor

davidlt commented Aug 29, 2013

It's in CMSSW_7_0_0_pre3.

@ktf
Copy link
Contributor

ktf commented Aug 29, 2013

Ok, perfect.@vadler can you try this again in today IB and see if it works now?

@vadler vadler mentioned this pull request Aug 29, 2013
@vadler
Copy link

vadler commented Aug 29, 2013

It does :-)
However, the file
CommonTools/ParticleFlow/python/Isolation/pfIsolatedElectrons_cfi.py
has been forgotten to be updated to the new GsfElectron interface in the topic, which breaks PFBRECO and the corresponding PAT integration tests.
I leave this one rejected and have re-queued it including the necessary fix as #666 (yes, the number of the beast!!!)
So, in principle this one can be closed.

@nclopezo
Copy link
Contributor

nclopezo commented Sep 2, 2013

Superseded by #680

@nclopezo nclopezo closed this Sep 2, 2013
@deguio deguio deleted the porting-changes-forBeaudett branch September 8, 2013 15:20
nclopezo pushed a commit to nclopezo/cmssw that referenced this pull request Nov 21, 2014
Add test for config.map schema.
parbol pushed a commit to parbol/cmssw that referenced this pull request Mar 5, 2015
Samples, AK8 jets, Status3 for Pythia8 (fixed version)
gdimperi pushed a commit to gdimperi/cmssw that referenced this pull request Jun 21, 2015
jpata pushed a commit to jpata/cmssw that referenced this pull request Sep 16, 2015
jbsauvan added a commit to jbsauvan/cmssw that referenced this pull request Nov 3, 2017
nadya-chernyavskaya pushed a commit to nadya-chernyavskaya/cmssw that referenced this pull request Apr 10, 2018
…integration

Add deep flavour of MiniAOD v2 to nanoaod
fwyzard pushed a commit to fwyzard/cmssw that referenced this pull request Sep 12, 2018
Add separate plots for tracks associated to the primary vertex.
bi-ran pushed a commit to bi-ran/cmssw that referenced this pull request Nov 7, 2018
* add some more hfCoincFilters for different thresholds

* changed the hf filter thr3 names [for clarity]
bi-ran pushed a commit to bi-ran/cmssw that referenced this pull request Jan 24, 2019
* add some more hfCoincFilters for different thresholds

* changed the hf filter thr3 names [for clarity]
slava77 pushed a commit to slava77/cmssw that referenced this pull request Oct 9, 2021
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.

9 participants