-
Notifications
You must be signed in to change notification settings - Fork 7
v2: Allow applying multiple conditions simultaneously #373
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| from collections.abc import Set | ||
| from dataclasses import dataclass, field | ||
| from enum import IntEnum | ||
| from itertools import chain | ||
| from pathlib import Path | ||
|
|
||
| import pandas as pd | ||
|
|
@@ -373,8 +374,10 @@ class CheckValidConditionTargets(ValidationTask): | |
| """Check that all condition table targets are valid.""" | ||
|
|
||
| def run(self, problem: Problem) -> ValidationIssue | None: | ||
| allowed_targets = set( | ||
| problem.model.get_valid_ids_for_condition_table() | ||
| allowed_targets = ( | ||
| set(problem.model.get_valid_ids_for_condition_table()) | ||
| if problem.model | ||
| else set() | ||
| ) | ||
| allowed_targets |= set(get_output_parameters(problem)) | ||
| allowed_targets |= { | ||
|
|
@@ -394,6 +397,28 @@ def run(self, problem: Problem) -> ValidationIssue | None: | |
| f"Condition table contains invalid targets: {invalid}" | ||
| ) | ||
|
|
||
| # Check that changes of simultaneously applied conditions don't | ||
| # intersect | ||
| for experiment in problem.experiment_table.experiments: | ||
| for period in experiment.periods: | ||
| if not period.condition_ids: | ||
| continue | ||
| period_targets = set() | ||
| for condition_id in period.condition_ids: | ||
| condition_targets = { | ||
| change.target_id | ||
| for cond in problem.condition_table.conditions | ||
| if cond.id == condition_id | ||
| for change in cond.changes | ||
| } | ||
|
Comment on lines
+408
to
+413
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this useful for tools? I guess tools might like some
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's already in a dict, the keys will be unique and we can't check for duplicates any. It works if we do the checking every time in such a function.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I rather meant like this. It checks all conditions at once though, rather than one at a time, but there's no issue with identifying duplicates. from collections import Counter
from itertools import chain
all_updates = [
problem.condition_table.get_updates(condition_id)
for condition_id in period.condition_ids
]
duplicates = {
target_id
for target_id, count in Counter(chain.from_iterable(all_updates)).items()
if count > 1
}Or a method that provides
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that it simplifies the code here, just that it might be useful to tools. But fine to keep as is until a tool dev requests something... |
||
| if invalid := (period_targets & condition_targets): | ||
| return ValidationError( | ||
| "Simultaneously applied conditions for experiment " | ||
| f"{experiment.id} have overlapping targets " | ||
| f"{invalid} at time {period.time}." | ||
| ) | ||
| period_targets |= condition_targets | ||
|
|
||
|
|
||
| class CheckUniquePrimaryKeys(ValidationTask): | ||
| """Check that all primary keys are unique.""" | ||
|
|
@@ -484,11 +509,14 @@ def run(self, problem: Problem) -> ValidationIssue | None: | |
| c.id for c in problem.condition_table.conditions | ||
| } | ||
| for experiment in problem.experiment_table.experiments: | ||
| missing_conditions = { | ||
| period.condition_id | ||
| for period in experiment.periods | ||
| if period.condition_id is not None | ||
| } - available_conditions | ||
| missing_conditions = ( | ||
| set( | ||
| chain.from_iterable( | ||
| period.condition_ids for period in experiment.periods | ||
| ) | ||
| ) | ||
| - available_conditions | ||
| ) | ||
|
Comment on lines
+512
to
+519
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing for here and below, is it useful to add this to the classes? Looks like a lot of additional interface though so fine as is problem.experiment_table.get_condition_ids()
problem.experiment_table.get_experiment(experiment_id).get_condition_ids()
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure yet. I think, usually one only needs the conditions for a single period, not for the full experiment at once. |
||
| if missing_conditions: | ||
| messages.append( | ||
| f"Experiment {experiment.id} requires conditions that are " | ||
|
|
@@ -646,12 +674,13 @@ class CheckUnusedConditions(ValidationTask): | |
| table.""" | ||
|
|
||
| def run(self, problem: Problem) -> ValidationIssue | None: | ||
| used_conditions = { | ||
| p.condition_id | ||
| for e in problem.experiment_table.experiments | ||
| for p in e.periods | ||
| if p.condition_id is not None | ||
| } | ||
| used_conditions = set( | ||
| chain.from_iterable( | ||
| p.condition_ids | ||
| for e in problem.experiment_table.experiments | ||
| for p in e.periods | ||
| ) | ||
| ) | ||
| available_conditions = { | ||
| c.id for c in problem.condition_table.conditions | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,8 +3,8 @@ | |
| from copy import deepcopy | ||
|
|
||
| from petab.v2 import Problem | ||
| from petab.v2.C import * | ||
| from petab.v2.lint import * | ||
| from petab.v2.models.sbml_model import SbmlModel | ||
|
|
||
|
|
||
| def test_check_experiments(): | ||
|
|
@@ -21,3 +21,19 @@ def test_check_experiments(): | |
| tmp_problem = deepcopy(problem) | ||
| tmp_problem["e1"].periods[0].time = tmp_problem["e1"].periods[1].time | ||
| assert check.run(tmp_problem) is not None | ||
|
|
||
|
|
||
| def test_check_incompatible_targets(): | ||
| """Multiple conditions with overlapping targets cannot be applied | ||
| at the same time.""" | ||
| problem = Problem() | ||
| problem.model = SbmlModel.from_antimony("p1 = 1; p2 = 2") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤯 Didn't know an SBML test model could be specified so simply.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💪 |
||
| problem.add_experiment("e1", 0, "c1", 1, "c2") | ||
| problem.add_condition("c1", p1="1") | ||
| problem.add_condition("c2", p1="2", p2="2") | ||
| check = CheckValidConditionTargets() | ||
| assert check.run(problem) is None | ||
|
|
||
| problem["e1"].periods[0].condition_ids.append("c2") | ||
| assert (error := check.run(problem)) is not None | ||
| assert "overlapping targets {'p1'}" in error.message | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need to add a
dropnaThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to avoid the overhead of the additional groupy / creating the additional dataframes. Usually there will only be a single condition ID.
I'd avoid the
uniqueotherwise this will hide errors in case of duplicated conditions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right, I thought it was some view instead
Yes, not sure why I did that...
No issue with your code but I find the groupby faster to understand.
Independently of this, again this might be something that tools want in a library function