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

feat: Enhance phys/root/filter functionality #3250

Merged
merged 7 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions phys/root/filter/test/Snakefile
Original file line number Diff line number Diff line change
@@ -1,15 +1,52 @@
rule define_columns:
rule filter_str:
input:
"ntuple0.root",
output:
"ntuple0_output.root",
"ntuple0_str_output.root",
log:
"logs/filter/filter.log",
"logs/filter/filter_str.log",
params:
input_tree_name="TestTree",
output_tree_name="TestTree",
criteria="(pt > 1400) && (pz > 19000)",
branches_to_save=["pz", "pt", "p"],
threads: 2
wrapper:
"master/phys/root/filter"

rule filter_list:
input:
"ntuple0.root",
output:
"ntuple0_list_output.root",
log:
"logs/filter/filter_list.log",
params:
input_tree_name="TestTree",
output_tree_name="TestTree",
criteria="pt > 1400",
criteria=["pt > 1400", "pz > 19000"],
branches_to_save=["pz", "pt", "p"],
verbose=True
threads: 2
wrapper:
"master/phys/root/filter"

rule filter_dict:
input:
"ntuple0.root",
output:
"ntuple0_dict_output.root",
log:
"logs/filter/filter.log",
params:
input_tree_name="TestTree",
output_tree_name="TestTree",
criteria={
"PT cut": "pt > 1400",
"PZ cut": "pz > 19000"
},
branches_to_save=["pz", "pt", "p"],
verbose=True
threads: 1
wrapper:
"master/phys/root/filter"
36 changes: 30 additions & 6 deletions phys/root/filter/wrapper.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,39 @@
__author__ = "Anfeng Li"
__author__ = "Anfeng Li, Jamie Gooding"
__copyright__ = "Copyright 2024, Anfeng Li"
__email__ = "[email protected]"
__email__ = "[email protected], [email protected]"
__license__ = "MIT"


from typing import Dict, List
import ROOT

ROOT.EnableImplicitMT(snakemake.threads)
if snakemake.threads > 1:
ROOT.EnableImplicitMT(snakemake.threads)
else:
ROOT.DisableImplicitMT()

# Parse criteria
_smk_criteria = snakemake.params.get("criteria", "true")
if isinstance(_smk_criteria, str):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't "true" be interpreted as a string filtering criteria with 'criterion = "true"; label = "true"? Is this intended?

Copy link
Contributor Author

@GoodingJamie GoodingJamie Sep 17, 2024

Choose a reason for hiding this comment

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

Ah, yes it will be! This is the behaviour in master currently—the only use case I can think of would be to copy a tree in one file to a differently named tree in another file, but there's rootcp for this (may be the subject of a PR in the near future!). So I think here it would be safe to change this and require that criteria is passed:

Suggested change
_smk_criteria = snakemake.params.get("criteria", "true")
_smk_criteria = snakemake.params.criteria

where a missing params.criteria should raise a TypeError when being converted to criteria and labels.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But isn't criteria optional? At least that is what it says on the meta.yaml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

criteria is optional currently, but there isn't a use case I can think of for not passing criteria which isn't already covered by rootcp (for which I've opened #3251). I'll update this in meta.yaml to reflect this

criteria = [_smk_criteria]
labels = [_smk_criteria]
elif isinstance(_smk_criteria, List):
criteria = _smk_criteria
labels = _smk_criteria
elif isinstance(_smk_criteria, Dict):
criteria = _smk_criteria.values()
labels = _smk_criteria.keys()
else:
raise TypeError("Parameter 'criteria' should be type of 'str', 'list' or 'dict'")

criteria = snakemake.params.get("criteria", "true")
branches_to_save = snakemake.params.get("branches_to_save", None)

df = ROOT.RDataFrame(snakemake.params.input_tree_name, snakemake.input[0])
df = df.Filter(criteria)
for criterion, label in zip(criteria, labels):
df = df.Filter(criterion, label)

if snakemake.params.get("verbose", False):
report = df.Report()

if branches_to_save is not None:
df.Snapshot(
snakemake.params.output_tree_name,
Expand All @@ -21,3 +42,6 @@
)
else:
df.Snapshot(snakemake.params.output_tree_name, snakemake.output[0])

if snakemake.params.get("verbose", False):
report.Print()
20 changes: 18 additions & 2 deletions test.py
Original file line number Diff line number Diff line change
Expand Up @@ -6829,10 +6829,26 @@ def test_root_define_columns():


@skip_if_not_modified
def test_root_filter():
def test_root_filter_str():
run(
"phys/root/filter",
["snakemake", "--cores", "2", "--use-conda", "-F"],
["snakemake", "--cores", "2", "--use-conda", "-F", "ntuple0_str_output.root"],
)


@skip_if_not_modified
def test_root_filter_list():
run(
"phys/root/filter",
["snakemake", "--cores", "2", "--use-conda", "-F", "ntuple0_list_output.root"],
)


@skip_if_not_modified
def test_root_filter_dict():
run(
"phys/root/filter",
["snakemake", "--cores", "2", "--use-conda", "-F", "ntuple0_dict_output.root"],
)


Expand Down
Loading