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

add bare tracker map in order to be able to make TPolyLine-based Strip detector maps #1

Merged
merged 1 commit into from
Jun 19, 2020

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Jun 5, 2020

title says it all.

@cmsbuild
Copy link

cmsbuild commented Jun 5, 2020

A new Pull Request was created by @mmusich (Marco Musich) for branch master.

@smuzaffar, @andrius-k, @kmaeshima, @schneiml, @mrodozov, @cmsbuild, @jfernan2, @fioriNTU, @tulamor can you please review it and eventually sign? Thanks.
@hdelanno, @fioriNTU, @jandrea, @idebruyn, @threus, @venturia this is something you requested to watch as well.
cms-bot commands are listed here

@mmusich
Copy link
Contributor Author

mmusich commented Jun 9, 2020

@smuzaffar, @andrius-k, @kmaeshima, @schneiml, @mrodozov, @jfernan2, @fioriNTU, @tulamor

what is the procedure to include this file in the data repo? It is not (yet) used in cmssw, but as soon as it will be available, I plan to consume it.

@mrodozov
Copy link

mrodozov commented Jun 9, 2020

Once you have the cmssw part that uses it, we test both PRs together until the cmssw part is using this data file in an acceptable way. the test doesn't require merge of this PR so don't worry about that

@mrodozov
Copy link

mrodozov commented Jun 9, 2020

and maybe have an extension for that file .dat .txt .json etc, whatever the format is

@mmusich
Copy link
Contributor Author

mmusich commented Jun 9, 2020

@mrodozov I'd actually like to have this file merged in the data-repo before providing the code in cmssw as there are already standalone application that can use it. Is that possible?
Also there are other files in the repo without any extentions. I'd rather leave it as it is.

@mrodozov
Copy link

mrodozov commented Jun 9, 2020

please test
new data files can be added as they don't break anything, yes.

@cmsbuild
Copy link

cmsbuild commented Jun 9, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/6898/console Started: 2020/06/09 13:25

@cmsbuild
Copy link

cmsbuild commented Jun 9, 2020

+1
Tested at: 44fee26
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c9c2f1/6898/summary.html
CMSSW: CMSSW_11_2_X_2020-06-08-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link

cmsbuild commented Jun 9, 2020

Comparison job queued.

@cmsbuild
Copy link

cmsbuild commented Jun 9, 2020

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c9c2f1/6898/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 36
  • DQMHistoTests: Total histograms compared: 2780497
  • DQMHistoTests: Total failures: 44
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2780403
  • DQMHistoTests: Total skipped: 50
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
  • Checked 152 log files, 16 edm output root files, 36 DQM output files

@mmusich
Copy link
Contributor Author

mmusich commented Jun 19, 2020

@smuzaffar, @andrius-k, @kmaeshima, @schneiml, @mrodozov, @jfernan2, @fioriNTU, @tulamor

This PR has been sitting idle for 10 days now, can we please move on with this it?
Please let me know if there is anything I can do.

@jfernan2
Copy link

@mmusich there is nothing DQM team can do about this PR, I am afraid cms-data belongs to Core software

@smuzaffar
Copy link
Contributor

As it is a new file so there is nothing blocking it. We just want to make sure that you are happy with the content of the changes by testing them in cmssw first. That is why we always ask for a test in cmssw for such changes.

@jfernan2 , consider cms-data repositories as part of cmssw packages. There is one to one mapping between cmssw package and cms-data repos. If DQM is responsible for DQM/SiStripMonitorClient cmssw package then they are also responsible for DQM-SiStripMonitorClient data repo. If you are happy with the change here then please sign it and we will integrate it for next IB.

@mmusich
Copy link
Contributor Author

mmusich commented Jun 19, 2020

@mmusich there is nothing DQM team can do about this PR, I am afraid cms-data belongs to Core software

@jfernan2 then why is the PR dqm-pending?
image

@jfernan2
Copy link

+1
I am sorry @smuzaffar @mmusich but I was not aware of this, however how can I say if I am happy with the change if it has not being tested and it is data file?
Moreover, I see that 90% of the PRs in cms-data have been closed without signing, could you explain me how can I know that this should be indeed signed?

@smuzaffar
Copy link
Contributor

90% might be too exaggerated :-) most of data externals are signed. Those, which are not signed directly on data external, are signed via corresponding cmssw PR (which was used to test it).

You should not sign it if there are no tests run. You should ask the question to auther what is the purpose if this and how this has been validation

@mmusich
Copy link
Contributor Author

mmusich commented Jun 19, 2020

@jfernan2 @smuzaffar
so if you really want me to release the CMSSW code, I can make the PR but I would still need to work on it, in the meanwhile you can look at it here: https://github.com/cms-sw/cmssw/compare/master...mmusich:TrackerRemapper_11_2_X?expand=1.
As I mentioned earlier there are certain standalone applications (outside of cmssw) that can use this, so that's why I wanted to make the file available first.
As for validation see please attached plot

image

which is obtained by running the cfg file example in the branch

@jfernan2
Copy link

@smuzaffar Yes, 90% may be exagerated globally :-) but the packages I glimpsed were 90%....
@mmusich if you are happy with the data file, so am I

@smuzaffar
Copy link
Contributor

+externals

@cmsbuild
Copy link

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2)

@smuzaffar
Copy link
Contributor

today's 11h00 IB will include this.

@mmusich
Copy link
Contributor Author

mmusich commented Jun 19, 2020

thanks @smuzaffar !

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.

5 participants