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

New L1Trigger/DTTriggerPhase2 Files #2

Merged
merged 2 commits into from May 13, 2021
Merged

Conversation

jaimeleonh
Copy link

Adding new files for the DT Phase2 TPs:

  • Theta TP shifts
  • Local to global coordinates transformation
  • New patterns for the PseudoBayes grouping method

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @jaimeleonh (Jaime León Holgado) for branch master.

@smuzaffar, @mrodozov, @kpedro88, @cmsbuild, @rekovic, @srimanob, @cecilecaillol can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @battibass this is something you requested to watch as well.
cms-bot commands are listed here

@srimanob
Copy link

Please test

@smuzaffar
Copy link
Contributor

@jaimeleonh , these are all new files, So I guess there is nothing in existing cmssw code to use/test ... right? Do you have any cmssw PR to use these new data files?

@jaimeleonh
Copy link
Author

@smuzaffar We opened this PR to cmssw this morning cms-sw/cmssw#33243

@smuzaffar
Copy link
Contributor

thanks @jaimeleonh . By the way, I do not see any direct ref to these createdPatterns*.root files in cms-sw/cmssw#33243 , so are these newly added createdPatterns*.root files really needed ?

@jaimeleonh
Copy link
Author

jaimeleonh commented Mar 22, 2021

@smuzaffar I think they will be used in a later PR to cmssw. @folguera can you confirm?

Edit: maybe they can be used instead of the file in this line: https://github.com/dtp2-tpg-am/cmssw/blob/AM_11_3_X_Integration/L1Trigger/DTTriggerPhase2/python/PseudoBayesGrouping_cfi.py#L3

@folguera
Copy link
Contributor

indeed @smuzaffar they will be useed in a soon-to-come PR

@cmsbuild
Copy link
Contributor

-1

Failed Tests: Build
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-74bde5/13659/summary.html
COMMIT: 7840e14
CMSSW: CMSSW_11_3_X_2021-03-21-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-data/L1Trigger-DTTriggerPhase2/2/13659/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

>> Checking EDM Class Version for src/DataFormats/CastorReco/src/classes_def.xml in libDataFormatsCastorReco.so
@@@@ ----> OK  EDM Class Version 
>> Checking EDM Class Transients in libDataFormatsAlignment.so
@@@@ ----> OK  EDM Class Version 
>> Checking EDM Class Transients in libDataFormatsBeamSpot.so
error: class 'CLHEP::Hep3Vector' has a different checksum for ClassVersion 10. Increment ClassVersion to 11 and assign it to checksum 3438801427
Suggestion: You can run 'scram build updateclassversion' to generate src/DataFormats/CLHEP/src/classes_def.xml.generated with updated ClassVersion
gmake: *** [tmp/slc7_amd64_gcc900/edm_checks/libDataFormatsCLHEP.so] Error 1
>> Checking EDM Class Version for src/DataFormats/Common/src/classes_def.xml in libDataFormatsCommon.so
Error in : Dictionary trigger function for DataFormatsCLHEP_xr not found
@@@@ ----> OK  EDM Class Version 


@jaimeleonh
Copy link
Author

jaimeleonh commented Apr 7, 2021

Hi @smuzaffar , could you have a look at this failed check? Is there anything we need to do from our side? Thanks!

@smuzaffar
Copy link
Contributor

please test
lets re-tests it based on latest IBs where newer clhep and related code has been integrated

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-74bde5/14062/summary.html
COMMIT: 7840e14
CMSSW: CMSSW_11_3_X_2021-04-06-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-data/L1Trigger-DTTriggerPhase2/2/14062/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2640868
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2640839
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: no differences found

@folguera
Copy link
Contributor

Hi @smuzaffar is there anything else pending, can this be merged? The PR that needs these files has already been merged centrally: cms-sw/cmssw#33243

@smuzaffar
Copy link
Contributor

+externals
@folguera it neds signatures from l1 and upgrade . You also mentioned that there will a new cmssw PR which will use these new files? Is that PR already available?

@folguera
Copy link
Contributor

@smuzaffar some of the files can be already used by the exising (and merged) PR (cms-sw/cmssw#33243) other files will be used by a future PR that still needs to be made.

@rekovic, @cecilecaillol can you have a look at this?

@cecilecaillol
Copy link

+l1

@folguera
Copy link
Contributor

folguera commented May 13, 2021

Hi @smuzaffar we are still missing some signatures, who else should we ping to merge this?

@mrodozov mrodozov merged commit 829904e into cms-data:master May 13, 2021
@mrodozov
Copy link
Contributor

@folguera this is only new files, correct ?

@folguera
Copy link
Contributor

yes, only new files.

@mrodozov
Copy link
Contributor

usually we don't merge data PRs if they are only new files not used by any code yet
I'll include the files in this night IB trough cms-sw/cmsdist#6903 , but if there is nothing in the release that is using the files - they are not needed in the release :)

@folguera
Copy link
Contributor

sorry I dind't expressed clearly, all of the files are used by some code already merged in CMSSW, however only a subset of these files are used by the default version of the phase-2 DT trigger primitive generation code. The others are used by extensions that we are working on and optimizing, but a v0 of them is already in CMSSW. All the files are new, they are not a modification of pre-existing files in this repo.

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