Lint mapping table#480
Conversation
| str_p | ||
| for p in overrides | ||
| if isinstance(p, sp.Symbol) | ||
| and (str_p := str(p)) not in condition_targets |
There was a problem hiding this comment.
Not necessary since this is collected as a set and the condition_targets are removed further down.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #480 +/- ##
==========================================
- Coverage 75.29% 75.28% -0.01%
==========================================
Files 62 62
Lines 6946 6976 +30
Branches 1229 1241 +12
==========================================
+ Hits 5230 5252 +22
- Misses 1245 1247 +2
- Partials 471 477 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
BSnelling
force-pushed
the
lint_mapping_table
branch
from
4df4635 to
e1c3d3d
Compare
dweindl
left a comment
dweindl
left a comment
There was a problem hiding this comment.
Thanks. Almost there, I think.
| #: Nominal value. | ||
| nominal_value: Annotated[ | ||
| float | None, BeforeValidator(_convert_nan_to_none) | ||
| float | Literal["array"] | None, BeforeValidator(_convert_nan_to_none) |
| messages = [] | ||
|
|
||
| # Mapping table is optional | ||
| if problem.mappings: |
There was a problem hiding this comment.
| messages = [] | |
| # Mapping table is optional | |
| if problem.mappings: | |
| # Mapping table is optional | |
| if problem.mappings: | |
| return None | |
| messages = [] | |
| ... |
Reduce indentation.
| petab_id = getattr(mapping, "petab_id", None) | ||
| model_id = getattr(mapping, "model_id", None) |
There was a problem hiding this comment.
In which situation is this preferable over just petab_id = mapping.petab_id?
| # Duplicates for annotation-only rows (identity mappings) | ||
| # are permitted. |
There was a problem hiding this comment.
By duplicates, you mean petab_id == model_id, not multiple rows with the same petab_id or model_id, right?
It's not super clear from the specs whether the latter would be allowed or not.
| old_petab_ids = ( | ||
| {c.id for c in problem.conditions} | ||
| | {e.id for e in problem.experiments} | ||
| | {o.id for o in problem.observables} |
There was a problem hiding this comment.
Also check for parameter table parameters?
| for mapping in problem.mappings: | ||
| if mapping.model_id and mapping.model_id in parameter_ids.keys(): | ||
| if mapping.petab_id not in invalid: | ||
| parameter_ids[mapping.petab_id] = None |
There was a problem hiding this comment.
This might add all kinds of entities to the allowed-in-parameter-table dict. Species, observables, experiments, ...
For the currently supported model types (SBML, PySB), I don't think there can be a situation where the mapping table would contribute additional values allowed in the parameter table. For a model type where this could happen, we'd need some API for accessing parameters that are allowed in the parameter table if they had proper IDs. This doesn't exist yet. So either we need to add it now, or completely ignore the mapping table in this function. (The same holds for get_required_parameters_for_parameter_table below.)
| # Start with mapping table petab ids | ||
| parameter_ids = {m.petab_id for m in problem.mappings} | ||
|
|
||
| # Add parameters from measurement table, unless they are fixed parameters |
There was a problem hiding this comment.
With the changes below, this is no longer true.
BSnelling
force-pushed
the
lint_mapping_table
branch
from
ef86844 to
d903f22
Compare
Co-authored-by: Daniel Weindl <dweindl@users.noreply.github.com>
5 participants
petab.v2.lint.get_valid_parameters_for_parameter_table: add mapping tablepetabEntityIdregardless of whether the correspondingmodelEntityIdwould be valid (was L937->L1036)