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

Be able to use merged magnetic field interpolation files #38051

Merged
merged 10 commits into from Jun 4, 2022

Conversation

Dr15Jones
Copy link
Contributor

PR description:

  • Provide a utility to create an interpolation file which is a merger of all files for a given table set. This also creates a separate index file which says where in the merge file each individual file is located.
  • Extend DD4Hep_MagGeoBuilder to be able to use either the merged file (if present) or the individual files
  • Use of the merged file decreased the setup time from 9 seconds to less than 2 seconds on my test machine.

PR validation:

  • Running the unit tests in MagneticField/Engine pass both with and without the index file being present
  • Used GridFileReader to dump the contents of the interpolation file with and without the index. The two files chosen gave identical dumped information.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38051/30134

  • This PR adds an extra 44KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

  • MagneticField/GeomBuilder (reconstruction)
  • MagneticField/Interpolation (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@namapane this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

please test

@namapane
Copy link
Contributor

Hi Chris, nice job. A couple of comments:
-If I understand correctly there is currently no way to force reading tables instead than the merged file? Once the merged file will be in the release there will be no way for to remove it, which may be needed for development.
-could you please add some comment on how mergeFieldTable is supposed to be called, possibly both in as comments in the cc and in the SWGuide?
By the time we will need to update the map the next time, we may have forgotten about this step otherwise...

@Dr15Jones
Copy link
Contributor Author

If I understand correctly there is currently no way to force reading tables instead than the merged file? Once the merged file will be in the release there will be no way for to remove it, which may be needed for development.

If you want, I could add a configuration parameter to override the default behavior and use the individual files.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54d031/24925/summary.html
COMMIT: 75ceeb4
CMSSW: CMSSW_12_5_X_2022-05-23-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38051/24925/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: 50
  • DQMHistoTests: Total histograms compared: 3650985
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3650955
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 49 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@namapane
Copy link
Contributor

If I understand correctly there is currently no way to force reading tables instead than the merged file? Once the merged file will be in the release there will be no way for to remove it, which may be needed for development.

If you want, I could add a configuration parameter to override the default behavior and use the individual files.

From a quick look it seems to me that percolating such a parameter would require an annoying number of changes, so my preference would be just to set a flag in the code for the purpose, that can be changed at compile time - as this would be used only for development, having to recompile one file would not be an issue in that case. Of course the developer may hack the code as well, but having flag ready will make the task more obvious and straightforward.

@Dr15Jones
Copy link
Contributor Author

From a quick look it seems to me that percolating such a parameter would require an annoying number of changes, so my preference would be just to set a flag in the code for the purpose

Turns out, I had written the code yesterday anyway so I've included it in the pull request. I appreciate the concern about how much effort it would take.

@cmsbuild
Copy link
Contributor

Pull request #38051 was updated. @jpata, @clacaputo, @slava77 can you please check and sign again.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54d031/24945/summary.html
COMMIT: c00fa60
CMSSW: CMSSW_12_5_X_2022-05-24-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38051/24945/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: 50
  • DQMHistoTests: Total histograms compared: 3650985
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3650955
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 49 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@Dr15Jones
Copy link
Contributor Author

The twiki documentation was updated with how to create the merged files.

@namapane
Copy link
Contributor

The twiki documentation was updated with how to create the merged files.

Cool, thanks Chris!

@clacaputo
Copy link
Contributor

Hi @Dr15Jones , out of curiosity, in order to use the merged file, one should run mergeFieldTable.cc producing the two files .merge and .index, right? What's the plan for using them in DD4hep_VolumeBasedMagneticFieldESProducer or DD4hep_VolumeBasedMagneticFieldESProducerFromDB?
Are this file gonna be produced and stored in the release?

@Dr15Jones
Copy link
Contributor Author

in order to use the merged file, one should run mergeFieldTable.cc producing the two files .merge and .index, right?

Correct. Those files need to be in the same directory as holds (or would hold) the present 9000+ files.

What's the plan for using them in DD4hep_VolumeBasedMagneticFieldESProducer or DD4hep_VolumeBasedMagneticFieldESProducerFromDB?

That would be up to the maintainers of those packages.

Are this file gonna be produced and stored in the release?

They are not in the release, per-se. They would go into the external data area same as where the non merged files live now.

@clacaputo
Copy link
Contributor

+reconstruction

  • technical PR
  • no reco changes

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 2, 2022

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Jun 3, 2022

please test
to refresh

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 3, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-54d031/25230/summary.html
COMMIT: c00fa60
CMSSW: CMSSW_12_5_X_2022-06-02-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38051/25230/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: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3649923
  • DQMHistoTests: Total failures: 14
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3649887
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@qliphy
Copy link
Contributor

qliphy commented Jun 4, 2022

+1

@cmsbuild cmsbuild merged commit 51e5a9a into cms-sw:master Jun 4, 2022
@namapane
Copy link
Contributor

namapane commented Jun 5, 2022

in order to use the merged file, one should run mergeFieldTable.cc producing the two files .merge and .index, right?

Correct. Those files need to be in the same directory as holds (or would hold) the present 9000+ files.

What's the plan for using them in DD4hep_VolumeBasedMagneticFieldESProducer or DD4hep_VolumeBasedMagneticFieldESProducerFromDB?

That would be up to the maintainers of those packages.

Are this file gonna be produced and stored in the release?

They are not in the release, per-se. They would go into the external data area same as where the non merged files live now.

Hi @Dr15Jones,
Publishing data files is done by a PR to https://github.com/cms-data/MagneticField-Interpolation . I can do this myself at some point, but given that you already have the files and done all the tests, maybe it's safer if you make the PR directly?

@Dr15Jones
Copy link
Contributor Author

Hi @namapane

I can do this myself at some point, but given that you already have the files and done all the tests, maybe it's safer if you make the PR directly?

Turns out, I've already deleted that development area (I do that frequently) so I don't have the files any longer.

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

5 participants