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

Consumes are restored in DiLepton code, conflict is resolved #4499

Merged
merged 5 commits into from Jul 19, 2014

Conversation

nadjieh
Copy link
Contributor

@nadjieh nadjieh commented Jul 3, 2014

Checked on
CMSSW_7_2_X_2014-07-01-0200
And everything looks ok.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 3, 2014

A new Pull Request was created by @nadjieh (Abideh Jafari) for CMSSW_7_2_X.

Consumes are restored in DiLepton code, conflict is resolved

It involves the following packages:

DQM/Physics

@ojeda, @danduggan, @rovere, @cmsbuild, @nclopezo, @deguio, @Degano can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 3, 2014

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 3, 2014

@deguio
Copy link
Contributor

deguio commented Jul 7, 2014

+1

@deguio
Copy link
Contributor

deguio commented Jul 7, 2014

-1
noticing only now. @nadjieh could you please address all the comments made in:
#4229

keeping this PR on hold for now.
thanks,
F.

@nadjieh
Copy link
Contributor Author

nadjieh commented Jul 8, 2014

Hi @deguio https://github.com/deguio,

What comments exactly?
If you are speaking about the memory problem, they are in:

[1]
nadjieh@f764a06#diff-47ebdbafe0b07b16dcf8273bae092780R457
[2]
https://github.com/nadjieh/cmssw/blob/f764a06dd034a08368c5d1046cdc43aaad5d1be6/DQM/Physics/src/SingleTopTChannelLeptonDQM.h#L52

The other comment was on "consumes" which is the main reason for this pull
request.
There was also a style correction in [2]

{delete pvSelect_;} ---> {delete pvSelect_;};

which I guess does not harm as it is.

Cheers,
Nadjieh

On 7 July 2014 17:03, deguio notifications@github.com wrote:

-1
noticing only now. @nadjieh https://github.com/nadjieh could you please
address all the comments made in:
#4229 #4229

keeping this PR on hold for now.
thanks,
F.


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

@deguio
Copy link
Contributor

deguio commented Jul 15, 2014

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes or unless it breaks tests.

@cmsbuild
Copy link
Contributor

Pull request #4499 was updated. @ojeda, @danduggan, @rovere, @cmsbuild, @nclopezo, @deguio, @Degano can you please check and sign again.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@deguio
Copy link
Contributor

deguio commented Jul 19, 2014

+1
seems to me that all the comments have been addressed.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

@ktf
Copy link
Contributor

ktf commented Jul 19, 2014

Merging this, however it seems to me the code has still problems. For example it's not clear to me what deletes jetIDSelect_ and other stuff which get's allocated via new.

ktf added a commit that referenced this pull request Jul 19, 2014
Consumes are restored in DiLepton code, conflict is resolved
@ktf ktf merged commit 5f23e47 into cms-sw:CMSSW_7_2_X Jul 19, 2014
@ktf
Copy link
Contributor

ktf commented Jul 19, 2014

I think this breaks a bunch of IBs tests:

https://cmssdt.cern.ch/SDT/cgi-bin//showMatrixTestLogs.py/slc6_amd64_gcc481/www/sat/7.2-sat-14/CMSSW_7_2_X_2014-07-19-1400/pyRelValMatrixLogs/run/

reverting it. Can you please look at what's wrong? Thanks.

@nadjieh
Copy link
Contributor Author

nadjieh commented Jul 21, 2014

Hi Giulio,

It seems that the method
trackerExpectedHitsInner
is deprecated in TrackBase.h

I am trying to figure out when it happened. Also I have to ask EGamma
contacts to see what we should use instead.

We have two ways:

  • Comment this requirement in the electron selection for the moment and
    integrate
  • Wait for answers from EG people.

What do you think?

Nadjieh

On 20 July 2014 00:32, Giulio Eulisse notifications@github.com wrote:

I think this breaks a bunch of IBs tests:

https://cmssdt.cern.ch/SDT/cgi-bin//showMatrixTestLogs.py/slc6_amd64_gcc481/www/sat/7.2-sat-14/CMSSW_7_2_X_2014-07-19-1400/pyRelValMatrixLogs/run/

reverting it. Can you please look at what's wrong? Thanks.


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

@ktf
Copy link
Contributor

ktf commented Jul 21, 2014

I do not have any input to give on the actual physics / DQM part, just
the technical side. @deguio is the one who should comment.

@deguio
Copy link
Contributor

deguio commented Jul 21, 2014

hello,
a new PR has to be opened anyway and we are not in a rush for the next pre release. I would wait for an answer from the EG people.
please keep us posted about this.

thank you,
F.

@nadjieh
Copy link
Contributor Author

nadjieh commented Jul 24, 2014

Hi,

Please check the new pull request:

#4782

Thanks,
N.

On 21 July 2014 13:58, deguio notifications@github.com wrote:

hello,
a new PR has to be opened anyway and we are not in a rush for the next pre
release. I would wait for an answer from the EG people.
please keep us posted about this.

thank you,
F.


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

smuzaffar added a commit that referenced this pull request Aug 1, 2014
Inherited from #4499: change the missingHit method to avoid crash
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

4 participants