Skip to content

Improve SoATemplate README - #51615

Merged
cmsbuild merged 1 commit into
cms-sw:masterfrom
Electricks94:ReworkSoAREADME
Aug 12, 2026
Merged

Improve SoATemplate README#51615
cmsbuild merged 1 commit into
cms-sw:masterfrom
Electricks94:ReworkSoAREADME

Conversation

@Electricks94

@Electricks94 Electricks94 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

PR description:

This PR improves the README of SoATemplate.

PR validation:

This PR is documentation only and does not change any code

fixes #51522

@felicepantaleo fyi

@cmsbuild

cmsbuild commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

cms-bot internal usage

@Electricks94

Copy link
Copy Markdown
Contributor Author

@chrishanw does this PR address all problems you had with the README of the SoATemplate?

@cmsbuild

cmsbuild commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51615/50418

@cmsbuild

cmsbuild commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

A new Pull Request was created by @Electricks94 for master.

It involves the following packages:

  • DataFormats/SoATemplate (heterogeneous)

@cmsbuild, @fwyzard, @makortel can you please review it and eventually sign? Thanks.
@missirol, @mmusich, @rovere this is something you requested to watch as well.
@ftenchini, @mandrenguyen, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@cmsbuild

cmsbuild commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51615/50420

@cmsbuild

cmsbuild commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Pull request #51615 was updated. @cmsbuild, @fwyzard, @makortel can you please check and sign again.

#include <catch2/catch_all.hpp>

#include "DataFormats/SoATemplate/interface/SoALayout.h"
#include "DataFormats/SoATemplate/interface/SoABlocks.h"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should include both ?
In general I find it god practice to include all usad headers explicitly, without relying on transitive inclusions.

struct ViewTemplateFreeParams; \
\
SOA_HOST_ONLY \
void soaToStreamInternal(std::ostream & _soa_impl_os) const { \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this tested anywhere ?
More in general, can you add an example of using this to the README ?

@fwyzard

fwyzard commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

@Electricks94 thanks for the update to the documentation.

Could you split the documentation changes and the code changes in two different commits ?

Could you reflow the README file to stay within 120 columns, where possible ?

Thanks!

@cmsbuild

Copy link
Copy Markdown
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51615/50543

@cmsbuild

Copy link
Copy Markdown
Contributor

Pull request #51615 was updated. @cmsbuild, @fwyzard, @makortel can you please check and sign again.

@cmsbuild

Copy link
Copy Markdown
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51615/50544

@cmsbuild

Copy link
Copy Markdown
Contributor

Pull request #51615 was updated. @cmsbuild, @fwyzard, @makortel can you please check and sign again.

@Electricks94

Copy link
Copy Markdown
Contributor Author

@Electricks94 thanks for the update to the documentation.

Could you split the documentation changes and the code changes in two different commits ?

Could you reflow the README file to stay within 120 columns, where possible ?

Thanks!

Accidently I misread the comment and pushed the code changes to a new PR... I am sorry for that! The code changes are now in #51670 and the changes to the README here.

I formatted the README file to stay within the 120 columns limit always

@fwyzard

fwyzard commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

No problem... hopefully the two PRs will not conflict.

@fwyzard

fwyzard commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

+heterogeneous

@cmsbuild

Copy link
Copy Markdown
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @sextonkennedy, @ftenchini, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2)

@fwyzard

fwyzard commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

please test

@fwyzard

fwyzard commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

@smuzaffar @akritkbehera do you think the bot could be developed so if the only changes in a PR are about documentation, the tests can be skipped?

For example if the only changes are in files with an extension of .md ?

@cmsbuild

Copy link
Copy Markdown
Contributor

+1

Size: This PR adds an extra 28KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8848d9/55254/summary.html
COMMIT: 1142486
CMSSW: CMSSW_20_1_X_2026-08-11-1100/el9_amd64_gcc13
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/51615/55254/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@mandrenguyen

Copy link
Copy Markdown
Contributor

+1

@cmsbuild
cmsbuild merged commit d6e7eb5 into cms-sw:master Aug 12, 2026
10 checks passed
@smuzaffar

Copy link
Copy Markdown
Contributor

@smuzaffar @akritkbehera do you think the bot could be developed so if the only changes in a PR are about documentation, the tests can be skipped?

For example if the only changes are in files with an extension of .md ?

sure @fwyzard , I will open cms-bot to do that

@smuzaffar

Copy link
Copy Markdown
Contributor

@smuzaffar @akritkbehera do you think the bot could be developed so if the only changes in a PR are about documentation, the tests can be skipped?

For example if the only changes are in files with an extension of .md ?

done via cms-sw/cms-bot#2824

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.

Review of the SoATemplate README

5 participants