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

Replace SMatrixNoInit #4066

Merged
merged 1 commit into from Jun 1, 2014
Merged

Conversation

wmtan
Copy link
Contributor

@wmtan wmtan commented May 31, 2014

ROOT6 has replaced SMatrixNoInit() with SMatrixIdentity().
This pull request adapts CMSSW to this change, and fixes most, perhaps all, of the compilation errors in the latest ROOT6 IB. Please merge ASAP.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @wmtan for CMSSW_7_2_ROOT6_X.

Replace SMatrixNoInit

It involves the following packages:

DataFormats/Math
RecoVertex/KinematicFit
TrackingTools/AnalyticalJacobians
TrackingTools/GsfTools
TrackingTools/TrajectoryParametrization

@nclopezo, @cmsbuild, @thspeer, @StoyanStoynev, @slava77, @Degano can you please review it and eventually sign? Thanks.
@gpetruc, @cerati, @GiacomoSguazzoni, @rovere, @venturia this is something you requested to watch as well.
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.

@ktf
Copy link
Contributor

ktf commented May 31, 2014

SMatrixNoInit was actually introduced in Root5 by @VinInn, to avoid unneeded initializations and speedup the Reco by ~3%. Since the patch still applies for root6, I've simply forward ported the patch. CMSSW_7_2_ROOT6_X_2014-31-05-0200 should have the change. I'll follow up with the root people to see if we can get it included in the first convenient patch release of root6.

@VinInn
Copy link
Contributor

VinInn commented May 31, 2014

please close this PR.
I am sorry for the confusion created in the root6 branch by the changes in the main trench

@ktf
Copy link
Contributor

ktf commented May 31, 2014

  1. The CMSSW_7_2_ROOT6_X_2014-31-05-0200 build failed, if I interpret
    Jenkins correctly.

No, it did not. I stopped it and restarted with the new patch applied.
It's still building.

  1. I need a working IB by Monday or i am dead in the water. If the
    problem with the build has anything to do with the SMatrixNoInit patch
    and it cannot be fixed by Monday, Fermilab time, I would request that
    this pull request be merged as a temporary workaround so I can get a
    good IB. We can easily back it out later when SMatrixNoInit is
    working.

I'll do my best to make sure you have something working by monday at the
latest, but hopefully in just a few hours.

  1. I need a copy of the root6 patch adding SMatrixNoInit()

cms-sw/root@a792dbb

@wmtan
Copy link
Contributor Author

wmtan commented May 31, 2014

The new patch reproducibly causes segfaults in genreflex during building over 30 packages.
Unless this can be fixed by Monday morning, Fermilab time, I request the backout of the patch and the merging of this pull request ASAP. I need a working IB. The 3% RECO performance improvement can wait until ROOT6 is functioning.

ktf added a commit that referenced this pull request Jun 1, 2014
@ktf ktf merged commit 5fd70f0 into cms-sw:CMSSW_7_2_ROOT6_X Jun 1, 2014
@ktf
Copy link
Contributor

ktf commented Jun 1, 2014

Ok, I reverted the patch and merged this (ROOT6) only. I'll check tomorrow why there were crashes.

@wmtan
Copy link
Contributor Author

wmtan commented Jun 1, 2014

Thanks. The crashes are reproducible. Unless they are solved first by someone else, I will show them to Philippe soon, and we can discuss how to proceed. In the meantime, this issue will not interfere with other ROOT6 work.

@wmtan
Copy link
Contributor Author

wmtan commented Jun 2, 2014

The patch was reverted, but the changes for this pull request are not in the latest IB, CMSSW_7_2_ROOT6_X_2014-06-01-2200. I hope they will be in the next IB.

@ktf
Copy link
Contributor

ktf commented Jun 2, 2014

Yes, 0200 should be the first one to have them.

@wmtan wmtan deleted the ReplaceSMatrixNoInit branch June 3, 2014 15:11
@slava77 slava77 mentioned this pull request Mar 11, 2015
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