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] Deprecate RooDataSet constructors other than universal constructor #17083

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

guitargeek
Copy link
Contributor

The RooDataSet constructors to construct a dataset from a part of an
existing dataset are deprecated and will be removed in ROOT 6.38. This
is to avoid interface duplication. One should use RooAbsData::reduce()
instead, or to change the weight column, the universal constructor with
the Import(), Cut(), and WeightVar() arguments.

The tutorials and tests were adjusted accordingly.

Also, fix a wrong comment in the rf403 tutorials: they said that the
existing dataset is changed to interpret a different column as the
weight, but actually a new dataset was created. The comment is fixed in
this commit.

Since it was already pretty close to being formatted according to our
clang-tidy style.
The `RooDataSet` constructors to construct a dataset from a part of an
existing dataset are deprecated and will be removed in ROOT 6.38. This
is to avoid interface duplication. One should use `RooAbsData::reduce()`
instead, or to change the weight column, the universal constructor with
the `Import()`, `Cut()`, and `WeightVar()` arguments.

The tutorials and tests were adjusted accordingly.

Also, fix a wrong comment in the `rf403` tutorials: they said that the
existing dataset is changed to interpret a different column as the
weight, but actually a new dataset was created. The comment is fixed in
this commit.
@@ -491,6 +491,14 @@
# define _R__DEPRECATED_636(REASON) _R_DEPRECATED_REMOVE_NOW(REASON)
#endif

/* USE AS `R__DEPRECATED(6,38, "Not threadsafe; use TFoo::Bar().")`
Copy link
Member

Choose a reason for hiding this comment

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

@dpiparo We should consider updating the release process documentation to include adding this macro right after updating the master for the new major release cycle.

Copy link

Test Results

    18 files      18 suites   4d 2h 45m 36s ⏱️
 2 682 tests  2 680 ✅ 0 💤 2 ❌
46 440 runs  46 438 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit 1dedf67.

Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

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

LGTM!
I agree with this deprecation

@guitargeek guitargeek merged commit cc99d62 into root-project:master Dec 3, 2024
17 of 21 checks passed
@guitargeek guitargeek deleted the roodataset_constructor branch December 3, 2024 18:34
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.

3 participants