Skip to content

Conversation

ferdymercury
Copy link
Collaborator

@ferdymercury ferdymercury commented Jul 1, 2025

This Pull request:

Changes or fixes:

Fixes https://its.cern.ch/jira/browse/ROOT-9330

This would be also a good excuse to revive #18090

Checklist:

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

@ferdymercury ferdymercury requested a review from couet July 1, 2025 17:35
@ferdymercury ferdymercury changed the title [test] add test for TFractionFitter [test] add test/tutorial for TFractionFitter Jul 1, 2025
@ferdymercury ferdymercury requested a review from guitargeek July 1, 2025 17:36
@ferdymercury ferdymercury marked this pull request as ready for review July 1, 2025 17:40
Copy link

github-actions bot commented Jul 1, 2025

Test Results

    21 files      21 suites   3d 7h 36m 57s ⏱️
 3 222 tests  3 220 ✅ 0 💤 2 ❌
65 943 runs  65 941 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit 14359e2.

♻️ This comment has been updated with latest results.

@dpiparo
Copy link
Member

dpiparo commented Jul 30, 2025

This PR is really good, thanks for proposing these changes. Maybe, and this is up to @martamaja10 also to say, the tutorial seems a bit old fashioned, with all the pointers, new and deletes and could use a slight modernisation...

@martamaja10
Copy link
Contributor

This PR is really good, thanks for proposing these changes. Maybe, and this is up to @martamaja10 also to say, the tutorial seems a bit old fashioned, with all the pointers, new and deletes and could use a slight modernisation...

Hi @ferdymercury,

Thanks a lot for this nice PR! I agree with Danilo, maybe some "modernisation" would be nice, so what it really involves is, for example changing lines like: TF1 *f0 = new TF1("f0", "[0]*(1-cos(x))/TMath::Pi()", 0., TMath::Pi()); to TF1 f0("f0", "[0]*(1-cos(x))/TMath::Pi()", 0., TMath::Pi()); and then we don't need the deletes either.

Cheers,
Marta

@martamaja10
Copy link
Contributor

Another thing that would be super super nice to add to this PR is an entry in the fit tutorials table which is defined in this file: https://github.com/root-project/root/blob/master/tutorials/math/fit/index.md. Thanks!

@ferdymercury ferdymercury requested a review from martamaja10 July 30, 2025 16:03
Copy link
Contributor

@martamaja10 martamaja10 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 for implementing my previous comments. Would it be possible to also squash the commits (we could have one for adding the new tutorial and one for adding the tests) and run a git clang-format at the end (I can see that the CI is complaining about the formatting: https://github.com/root-project/root/actions/runs/16625830587/job/47041863011?pr=19243)

Copy link
Contributor

@martamaja10 martamaja10 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 for this nice PR!

@martamaja10
Copy link
Contributor

Hi again @ferdymercury, sorry for being picky, one very very last thing before merging, the clang-format still complains. The two new files after clang-formatting should look like the ones under this link: https://cernbox.cern.ch/s/RzAxbAJbAVb60Io

taken from https://its.cern.ch/jira/browse/ROOT-9330

[test] add new test to CMakeList

[test] proper memory cleanup

[test] fix test failure

[test] not needed warning capture?

[test] update test catches

[test] avoid the use of new and delete

modernize as suggested by danilo and marta
Fixes https://its.cern.ch/jira/browse/ROOT-9330

[tutorial] avoid use of new and delete

as suggested by marta and danilo
and some formatting

[tutorials] add fraction-fitter tutorial to Hist1D-Fit table

as suggested by marta

[nfc] mention tutorial in class documentation
[nfc] mention roofit alternative
@ferdymercury
Copy link
Collaborator Author

the clang-format still complains.

My bad, I totally forgot that step :)

Fixed now.

@martamaja10 martamaja10 merged commit 8a687f5 into root-project:master Jul 31, 2025
24 of 26 checks passed
@ferdymercury ferdymercury deleted the patch-12 branch July 31, 2025 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants