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

[RF] assorted improvements #15499

Merged
merged 4 commits into from May 15, 2024

Conversation

egpbos
Copy link
Contributor

@egpbos egpbos commented May 13, 2024

This Pull request:

Changes or fixes:

Each of the four commits contains an independent improvement that I made while working on something else (error handling). They are not necessary, but I think they are useful nonetheless, so here they are.

  1. MultiProcess::Messenger worker to master messages implemented slightly more efficiently.
  2. Implemented full TestStatistics::RooAbsL::constOptimizeTestStatistic; these features are used rarely in Higgs fits, I think, but they are nonetheless features in non-MP RooFit fits that I now added for feature parity.
  3. Extend RooAbsL test suite with comparisons to the non-MP RooFit fits (RooNLLVar) that they were intended to reimplement.
  4. Extend NaNPacker test suite with some simple arithmetic demonstrations / sanity checks.

Checklist:

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

Use the same forwarding reference and ZeroMQ message queuing implemenation for worker to master (W2M) messages that was already used for master to worker (M2W) as well. The forwarding reference omits unnecessary copies and the queuing makes sending more robust by only using one receive call for the whole message.
The previous implementation was a stub that didn't cover all opcodes that can be used during minimization. This commit copies them over from the old RooAbsOptTestStatistic implementation.
Since RooAbsL intends to reimplement RooNLLVar, its test suite should compare. This commit adds those comparisons.
I wrote this small test just to make sure arithmetic worked as expected on what at first I thought was a rather exotic class. I think in the end it also serves as a demonstration of how cool and useful this class is.
Copy link

Test Results

    10 files      10 suites   2d 1h 27m 30s ⏱️
 2 635 tests  2 634 ✅ 0 💤 1 ❌
24 868 runs  24 867 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit e00aded.

@egpbos
Copy link
Contributor Author

egpbos commented May 14, 2024

CI failure (roottest-python-basic-datatype) is unrelated to this PR.

@guitargeek
Copy link
Contributor

Thank you very much for the improvements, and also for the code formatting!

@guitargeek guitargeek merged commit eb7d819 into root-project:master May 15, 2024
12 of 14 checks passed
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.

None yet

2 participants