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.

Sign up for GitHub

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

Jump to bottom

[RF] Documentation Changes requested by @guitargeek for the "RooFit AD Support" doc #14018

Merged
merged 4 commits into from
Nov 22, 2023
Merged

[RF] Documentation Changes requested by @guitargeek for the "RooFit AD Support" doc #14018

merged 4 commits into from
Nov 22, 2023

Conversation

QuillPusher
Copy link
Contributor

These changes are on top of the first draft that I submitted, that was merged as part of PR13929.

The requested changes can be tracked in my personal drafts repo here.

Changes:

  • RooFit intro before AD support -> already covered in "Overview on RooFit implementation details..." section by @guitargeek

  • Extracting free functions to a separate header file is optional, re-wrote it as one of the two currently recommended approaches

  • changed to "Support Code Generation" instead of "AD Support" in most places

  • minor formatting and spelling errors changed

This Pull request:

Changes or fixes:

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 4, 2023 •
edited
Loading

Test Results

         9 files           9 suites   1d 13h 19m 39s ⏱️
  2 478 tests   2 478 ✔️ 0 💤 0 ❌
21 350 runs  21 350 ✔️ 0 💤 0 ❌

Results for commit 400f88d.

♻️ This comment has been updated with latest results.

@guitargeek guitargeek self-assigned this Nov 5, 2023
@guitargeek guitargeek changed the title Documentation Changes requested by @guitargeek for the "RooFit AD Support" doc [RF] Documentation Changes requested by @guitargeek for the "RooFit AD Support" doc Nov 5, 2023
@QuillPusher
Copy link
Contributor Author

Thanks @guitargeek ,

  • added fully qualified function names in some places for better Doxygen rendering (I wasn't too sure about this, please review)

  • Removed random commit IDs and added branch name to repo links

@vgvassilev
Copy link
Member

@guitargeek ping. Is there anything else to be done here?

guitargeek
guitargeek approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Thank you very much for implementing the suggestions and addressing the comments!

I have reorganized the commit message a bit with an improved commit massage for your commit, and added some other commits with general documentation and code improvements to be merged together when the CI is green.

@guitargeek
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

QuillPusher and others added 3 commits November 22, 2023 00:22
@QuillPusher @guitargeek
[RF] Address change requests by Jonas in the RooFit AD support document
abbbe3a 
These changes are on top of the first draft that I submitted, that was
merged as part of
[PR13929}(root-project#13929).

The requested changes can be tracked in my personal drafts repo
[here](https://github.com/QuillPusher/QP_Drafts/pull/3).

Changes:

- RooFit intro before AD support -> already covered in "Overview on
  RooFit implementation details..." section by @guitargeek

- **Extracting free functions** to a separate header file is
  **optional**, re-wrote it as one of the two currently recommended
  approaches

- changed to "Support **Code Generation**" instead of "AD Support" in
  most places

- minor formatting and spelling errors changed

- added fully qualified function names in some places for better Doxygen
  rendering

- Removed random commit IDs and added branch name to repo links
@guitargeek
[RF] Move RooUnitTest out of public interface
ae52965 
The `RooUnitTest` is not meant to be part of the public user inferface,
it is only used for internal tests.
@guitargeek
[RF] RooFit documentation improvements
34bb32a 
Most importantly, specify what "RooFit" should link to in some cases:
the main RooFit documentation instead of the namespace documentation.
@guitargeek
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2023-11-21T23:31:42.267Z] /home/sftnight/build/workspace/root-pullrequests-build/root/roofit/roofitcore/src/RooDataHist.cxx:874:1: warning: base class ‘class RooDirItem’ should be explicitly initialized in the copy constructor [-Wextra]

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-performance-centos8-multicore/soversion.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2023-11-21T23:36:05.529Z] /data/sftnight/workspace/root-pullrequests-build/root/roofit/roofitcore/src/RooDataHist.cxx:874:1: warning: base class ‘class RooDirItem’ should be explicitly initialized in the copy constructor [-Wextra]

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2023-11-21T23:30:23.536Z] /home/sftnight/build/workspace/root-pullrequests-build/root/roofit/roofitcore/src/RooDataHist.cxx:874:1: warning: base class ‘class RooDirItem’ should be explicitly initialized in the copy constructor [-Wextra]

@guitargeek
Copy link
Contributor

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@guitargeek guitargeek merged commit cd55619 into root-project:master Nov 22, 2023
10 of 15 checks passed
@QuillPusher QuillPusher deleted the RooFit_AD_Doc_ReviewChanges branch November 24, 2023 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guitargeek guitargeek guitargeek approved these changes

@lmoneta lmoneta Awaiting requested review from lmoneta lmoneta is a code owner

@hageboeck hageboeck Awaiting requested review from hageboeck

@bellenot bellenot Awaiting requested review from bellenot bellenot is a code owner

@couet couet Awaiting requested review from couet couet is a code owner

Assignees

@guitargeek guitargeek

Labels
in:Documentation in:RooFit/RooStats new contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@QuillPusher @phsft-bot @vgvassilev @guitargeek