Functions for adding conditions/observables/parameter to Problem #328

dweindl · 2024-12-04T09:22:08Z

Add functions for adding individual conditions/observables/parameter/measurements to Problem. This will simplify writing test cases and interactively assembling petab problems.

petab.v2.Problem.add_* will be added / updated to the new format separately.

Related to #220.

codecov-commenter · 2024-12-04T09:32:15Z

Codecov Report

Attention: Patch coverage is 53.73134% with 62 lines in your changes missing coverage. Please review.

Project coverage is 74.26%. Comparing base (d3e4006) to head (34ebde1).

Files with missing lines	Patch %	Lines
petab/v2/problem.py	51.61%	16 Missing and 14 partials ⚠️
petab/v1/problem.py	51.72%	15 Missing and 13 partials ⚠️
petab/v2/petab1to2.py	71.42%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #328      +/-   ##
===========================================
- Coverage    74.86%   74.26%   -0.60%     
===========================================
  Files           52       52              
  Lines         4921     5048     +127     
  Branches       847      888      +41     
===========================================
+ Hits          3684     3749      +65     
- Misses         928      962      +34     
- Partials       309      337      +28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Will simplify writing test cases and interactively assembling petab problems. To be extended. Related to PEtab-dev#220.

dilpath

I think the new code here might already be handled by the old code, so I left a comment to discuss that suggestion.

dilpath · 2024-12-09T11:08:20Z

petab/v1/problem.py

@@ -1005,3 +1006,177 @@ def n_priors(self) -> int:
            return 0

        return self.parameter_df[OBJECTIVE_PRIOR_PARAMETERS].notna().sum()
+
+    def add_condition(self, id_: str, name: str = None, **kwargs):


To match column headers/C.py? Not exactly the same due to underscores... but we could decide whether to go for consistent or context-specific IDs everywhere.

Suggested change

def add_condition(self, id_: str, name: str = None, **kwargs):

def add_condition(self, condition_id: str, condition_name: str = None, **kwargs):

Alternative: change all of these add_* methods to take **kwargs that are used to create a pd.Series, which is then validated and concatenated. get_*_df can be used to set the index. Then no table-specific code, and no need to redefine column names here?

if not isinstance(kwarg, str) and isinstance(kwarg, list): kwarg = PARAMETER_SEPARATOR.join(map(str, kwarg))

Yeah, I was struggling with what would be preferable. In add_condition, it feels redundant to prefix everything with condition_. Then again, it might be considered confusing if the arguments don't match the table columns. For me, the former felt more important. Also with regards to potentially introducing a proper object model, I think we'd want things to be more pythonic and less petaby.

I don't think the kwargs-solution would be very convenient. That would mean, you'd have write add_observable(**{petab.OBSERVABLE_ID:"foo", petab.SIMULATION_CONDITION_ID: "bar"}), I think then I rather directly go back to constructing dataframes.

I'll leave it as is for. As discussed elsewhere, the v2 API is likely to change drastically overall where these points will be addressed.

Alright, fine for v1, we can revisit v2 in case we change the column names there

dilpath · 2024-12-09T11:09:12Z

petab/v1/problem.py

+        id_: str,
+        formula: str | float | int,
+        noise_formula: str | float | int = None,
+        noise_distribution: str = None,
+        transform: str = None,
+        name: str = None,


dilpath · 2024-12-09T11:14:59Z

petab/v1/problem.py

+        tmp_df = pd.DataFrame(record).set_index([OBSERVABLE_ID])
+        if self.observable_df is None:
+            self.observable_df = tmp_df
+        else:
+            self.observable_df = pd.concat([self.observable_df, tmp_df])


pd.concat docs:

Any None objects will be dropped silently unless they are all None in which case a ValueError will be raised.

Suggested change

tmp_df = pd.DataFrame(record).set_index([OBSERVABLE_ID])

if self.observable_df is None:

self.observable_df = tmp_df

else:

self.observable_df = pd.concat([self.observable_df, tmp_df])

tmp_df = pd.DataFrame(record).set_index([OBSERVABLE_ID])

self.observable_df = pd.concat([self.observable_df, tmp_df])

There is still some unnecessary things happening in pandas before this not-None object is returned. Then again, these functions here aren't so much about efficiency. I will think about it, thanks for the suggestion.

dilpath · 2024-12-09T11:37:25Z

petab/v2/petab1to2.py

+    if src == dest:
+        return


Is this to skip computation or avoid an error? If the latter, then src/dest could be relative, so Path.resolve() should be used first.

It's the latter. Agreed, resolve would be preferable. I didn't use it, because src/dest could be URLs. But on second thought, shutil.copy would break on those anyways...

dweindl self-assigned this Dec 4, 2024

dweindl force-pushed the feature_220_modify_problem branch from 4d5e790 to dc20a0c Compare December 5, 2024 10:44

dweindl marked this pull request as ready for review December 5, 2024 10:49

dweindl requested a review from a team as a code owner December 5, 2024 10:49

dweindl force-pushed the feature_220_modify_problem branch from dc20a0c to 78e32c0 Compare December 6, 2024 10:35

Functions for adding conditions/observables/parameter to Problem

6b9bff5

Will simplify writing test cases and interactively assembling petab problems. To be extended. Related to PEtab-dev#220.

dweindl force-pushed the feature_220_modify_problem branch from 78e32c0 to 6b9bff5 Compare December 6, 2024 11:56

dilpath reviewed Dec 9, 2024

View reviewed changes

dweindl force-pushed the feature_220_modify_problem branch from 47534b1 to b9fc128 Compare December 9, 2024 13:05

resolve paths

34ebde1

dweindl force-pushed the feature_220_modify_problem branch from b9fc128 to 34ebde1 Compare December 9, 2024 13:23

dilpath approved these changes Dec 9, 2024

View reviewed changes

dweindl merged commit 9baf981 into PEtab-dev:develop Dec 9, 2024
7 checks passed

dweindl deleted the feature_220_modify_problem branch December 9, 2024 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Functions for adding conditions/observables/parameter to Problem #328

Functions for adding conditions/observables/parameter to Problem #328

dweindl commented Dec 4, 2024 •

edited

Loading

codecov-commenter commented Dec 4, 2024 •

edited

Loading

dilpath left a comment

dilpath Dec 9, 2024

dilpath Dec 9, 2024

dweindl Dec 9, 2024

dweindl Dec 9, 2024

dilpath Dec 9, 2024

dilpath Dec 9, 2024

dilpath Dec 9, 2024

dweindl Dec 9, 2024

dilpath Dec 9, 2024

dweindl Dec 9, 2024

dweindl Dec 9, 2024

	def add_condition(self, id_: str, name: str = None, **kwargs):
	def add_condition(self, condition_id: str, condition_name: str = None, **kwargs):

Functions for adding conditions/observables/parameter to Problem #328

Functions for adding conditions/observables/parameter to Problem #328

Conversation

dweindl commented Dec 4, 2024 • edited Loading

codecov-commenter commented Dec 4, 2024 • edited Loading

Codecov Report

dilpath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dweindl commented Dec 4, 2024 •

edited

Loading

codecov-commenter commented Dec 4, 2024 •

edited

Loading