Skip to content

Conversation

jkgoodrich
Copy link
Contributor

I still need to add tests for the added function and modified function

@jkgoodrich jkgoodrich requested a review from klaricch August 27, 2024 13:46
only_group = [{"group": "adj"}, {"group": "raw"}]
group_gen_anc_a = [{"group": "adj", "gen_anc": "a"}]
group_gen_anc_a_b = [*group_gen_anc_a, {"group": "adj", "gen_anc": "b"}]
group_gen_anc = [*group_gen_anc_a_b, {"group": "adj", "gen_anc": "c"}]
Copy link
Contributor

Choose a reason for hiding this comment

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

should this one be named "group_gen_anc_a_b_c" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

after reading through, maybe just add a comment here to clarify since it breaks the pattern

group_sex = [{"group": "adj", "sex": "XX"}, {"group": "adj", "sex": "XY"}]
group_subset = [
{"group": "adj", "subset": "s1"},
{"group": "raw", "subset": "s1"},
Copy link
Contributor

@klaricch klaricch Sep 6, 2024

Choose a reason for hiding this comment

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

just following the patterns of the others, should this actually be {"group": "adj", "subset": "s2"}?

Copy link
Contributor

Choose a reason for hiding this comment

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

or add comment to clarify

{"group": "adj", "gen_anc": "b", "sex": "XY"},
]
group_gen_anc_a_b_sex = group_gen_anc_a_sex + group_gen_anc_b_sex
group_gen_anc_sex = [
Copy link
Contributor

Choose a reason for hiding this comment

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

group_gen_anc_a_b_c_sex?

Copy link
Contributor

Choose a reason for hiding this comment

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

add comment

*group_gen_anc_a_subset,
{"group": "adj", "gen_anc": "b", "subset": "s1"},
]
group_gen_anc_subset = [
Copy link
Contributor

Choose a reason for hiding this comment

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

group_gen_anc_a_b_c_subset?

Copy link
Contributor

Choose a reason for hiding this comment

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

add comment

group_gen_anc_a_b_sex_subset = (
group_gen_anc_a_sex_subset + group_gen_anc_b_sex_subset
)
group_gen_anc_sex_subset = [
Copy link
Contributor

Choose a reason for hiding this comment

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

group_gen_anc_a_b_c_sex_subset

Copy link
Contributor

Choose a reason for hiding this comment

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

add comment

combine_operator: str,
exact_match: bool,
expected: str,
metadata_combinations: Any,
Copy link
Contributor

Choose a reason for hiding this comment

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

what does metadata_combinations refer to?

expected_meta: List[Dict[str, str]],
expected_expr1: List[int],
expected_expr2: List[int],
) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

add params

(s_ga_list, False, *all_and, False, "no_sex_and_no_gen_anc"),
(s_ga_ex, True, *all_and, False, "no_sex_and_no_gen_anc"),
(["sex", "subset"], True, "or", "and", "and", False, "sex_or_subset"),
(ss_d_list, False, *all_and, False, "no_subset_and_no_downsampling"),
Copy link
Contributor

Choose a reason for hiding this comment

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

should only exclude if both subset and downsampling are present?

exact_match: bool,
expected_meta: str,
metadata_combinations: Any,
) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

add params

filtering criteria for keys/key-value pairs to exclude.
:param combine_operator: Whether to use "and" or "or" to combine the keep and
exclude filtering criteria.
:param exact_match: Whether to apply the filtering only to items with exactly the
Copy link
Contributor

Choose a reason for hiding this comment

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

clarify that this only relates to the "keep" items

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.

2 participants