v2: Don't merge tables when creating Problem
#405
Conversation
PEtab allows spreading conditions/observables/measurements/... across multiple tables. So far, the different tables of a certain type are merged when creating a `Problem`. This is convenient for simulation, but pretty inconvenient when loading/modifying/saving the problem, where one usually wants to maintain the old structure.
This replaces `Problem.${type}_table: ${type}Table` by `Problem.${type}_tables: list[${type}Table]` table and introduces a `Problem.${type}` property that combines them on demand.
Closes PEtab-dev#404.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #405 +/- ##
==========================================
- Coverage 74.18% 73.86% -0.32%
==========================================
Files 62 62
Lines 6794 6835 +41
Branches 1184 1199 +15
==========================================
+ Hits 5040 5049 +9
- Misses 1306 1328 +22
- Partials 448 458 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Overall looks good 👍
I am not sure about the naming of e.g. problem.conditions. Since it's read-only, a getter method, e.g. problem.get_conditions() seems more appropriate, so users are less likely to make the mistake of trying to set condition values with the output from problem.conditions. This might be a problem for older users especially, who are used to a single table.
petab/v2/problem.py
Outdated
| def experiment_df(self) -> pd.DataFrame | None: | ||
| """Experiment table as DataFrame.""" | ||
| return self.experiment_table.to_df() if self.experiment_table else None | ||
| experiments = self.experiments |
There was a problem hiding this comment.
That requires building the list twice. Can be changed to an assignment expression, though.
| If there are more than one condition tables, the condition | ||
| is added to the last one. |
petab/v2/problem.py
Outdated
| if not self.mapping_tables: | ||
| self.mapping_tables.append(core.MappingTable(mappings=[])) | ||
| self.mapping_tables[-1].mappings.append( |
There was a problem hiding this comment.
As self.add_mapping? But model_id is required there.
I find that unnecessarily verbose. I think, Python users should not be too surprised about any read-only properties. Any decent IDE with static type-checking will flag such a mistake. |
A user might take the dataframe and start editing it without realizing that it doesn't edit the actual problem. e.g. |
Do you mean I'd be fine with removing the latter completely... |
Sorry, makes sense now, I edited my last comment. Yes, |
3 participants
PEtab allows spreading conditions/observables/measurements/... across multiple tables. So far, the different tables of a certain type are merged when creating a
Problem. This is convenient for simulation, but pretty inconvenient when loading/modifying/saving the problem, where one usually wants to maintain the old structure.This replaces
Problem.${type}_table: ${type}TablebyProblem.${type}_tables: list[${type}Table]table and introduces aProblem.${type}property that combines them on demand.Closes #404.