Repository navigation
Add options to omit empty or default columns from v2 tables - #543
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #543 +/- ##
==========================================
+ Coverage 77.77% 77.99% +0.22%
==========================================
Files 67 68 +1
Lines 7708 7786 +78
Branches 1380 1389 +9
==========================================
+ Hits 5995 6073 +78
Misses 1226 1226
Partials 487 487 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
11c6e96 to
f6635e6
Compare
| continue | ||
| default = field.get_default(call_default_factory=True) | ||
| if all(getattr(row, name) == default for row in self.elements): | ||
| result.append(field.alias or name) |
There was a problem hiding this comment.
Should field.alias or name default to the other? Since field.alias or name appears twice.
| #: Parameter name. | ||
| name: Annotated[str | None, BeforeValidator(_convert_nan_to_none)] = Field( | ||
| alias=C.PARAMETER_NAME, default=None | ||
| ) |
There was a problem hiding this comment.
Why is it useful to do this? We should always keep columns that are not covered by the standard -- why treat parameterName specially?
There was a problem hiding this comment.
parameterName is covered by the standard. Not great, but it's there. And it's unclear how any conflict with annotations through the mapping table would be resolved.
There was a problem hiding this comment.
Oh true, somehow I thought we dropped it.
| PARAMETER_ID, | ||
| LOWER_BOUND, | ||
| UPPER_BOUND, | ||
| NOMINAL_VALUE, |
There was a problem hiding this comment.
nominalValuewas added toC.PARAMETER_DF_REQUIRED_COLS, as the spec requires the column to be present.
It is optional if estimate is true. Might be problematic to expect it always.
There was a problem hiding this comment.
It's not marked [nominalValue] in the specs, so my understanding is that the column is mandatory, but the respective field may be empty. Other columns, such as observableParameters, explicitly state the if all-empty, the column can be omitted. For nominalValue this isn't the case. While I'd say your interpretation is desirable, I don't think this what the specs say.
There was a problem hiding this comment.
Ah yes, I see your point. Also applies to upper and lower bound.
Do you know whether your description is the intended interpretation while writing the spec, or just a grey area we forgot to clarify? If the latter then I think it's fine to update the spec now, just like we updated the v1 spec when we realized some things were unclear.
There was a problem hiding this comment.
I will add that to PEtab-dev/PEtab#681, so it can be discussed. Generally, I am in favor of dropping it if all-empty. For the bounds it's unlikely that they would ever be all-empty, because that would mean there is no parameter to estimate at all. Allowed, I guess. Useful? Not sure.
There was a problem hiding this comment.
Since multiple parameter tables are supported, one could keep their fixed parameters in a separate table to their estimated ones. Then it's slightly nicer to not have columns related to estimation, in the fixed parameters table.
There was a problem hiding this comment.
True, I didn't consider this use case.
Converting PEtab v2 tables to DataFrames or TSV files currently always includes all standard columns, even if they are empty in every row. Add `TableColumns` modes `all` (default, current behavior), `non_empty`, and `non_default` to select the columns to include. Required columns (`C.*_DF_REQUIRED_COLS`) are always included. The mode can be passed to `to_df`, `to_tsv`, and `Problem.to_files`, or set via `petab.v2.set_options` or temporarily via `petab.v2.option_context`, which also applies to `Problem.*_df`. Also add the `parameterName` column as `Parameter.name`, add `nominalValue` to the required parameter table columns as in the spec, and make `to_df` work for empty parameter, observable, and measurement tables. Closes PEtab-dev#523. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
3 participants
Converting PEtab v2 tables to DataFrames or TSV files currently always includes all standard columns, even if they are empty in every row (#523). This adds three column modes via
petab.v2.TableColumns:all: all columns (default, unchanged behavior)non_empty: omit columns that are empty in every rownon_default: additionally omit columns that have their default value in every row (e.g.,noiseDistribution=normal)Required columns are always included. The mode can be passed explicitly, or set globally or temporarily, which also applies to the
Problem.*_dfproperties:Further changes:
parameterNameis now a field ofParameter(Parameter.name) instead of an extra attribute.C.*_DF_REQUIRED_COLS.nominalValuewas added toC.PARAMETER_DF_REQUIRED_COLS, as the spec requires the column to be present.to_dfnow works for empty parameter, observable, and measurement tables.Closes #523.
🤖 Generated with Claude Code