v2: Adapt to long conditions table

[dweindl]

Update creation of PEtab problems, validation, conversion from v1 to v2,... to the new long condition table.

Many TODOs remain. To be continued after #337.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 78.33333% with 39 lines in your changes missing coverage. Please review.

Project coverage is 74.58%. Comparing base (b83e345) to head (949d322).
Report is 43 commits behind head on develop.

Files with missing lines	Patch %	Lines
petab/v2/lint.py	62.66%	15 Missing and 13 partials ⚠️
petab/v2/conditions.py	84.61%	2 Missing and 2 partials ⚠️
petab/v2/problem.py	55.55%	3 Missing and 1 partial ⚠️
petab/v2/petab1to2.py	95.55%	1 Missing and 1 partial ⚠️
petab/v2/_helpers.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #339      +/-   ##
===========================================
- Coverage    74.65%   74.58%   -0.07%     
===========================================
  Files           54       56       +2     
  Lines         5381     5517     +136     
  Branches       923      958      +35     
===========================================
+ Hits          4017     4115      +98     
- Misses        1012     1036      +24     
- Partials       352      366      +14     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dilpath]

Thanks, looks good! There is a FIXME, fine to merge if that's intentional.

[dilpath]

So far it's looked backwards compatible, is this too?

[dweindl]

I am not 100% sure if the old check ever made sense. If a parameter is overridden for some condition, it is no longer allowed to be in the parameters table. Not sure what the original intention was.

[dilpath]

The original code seems to do that; it removes parameters that are overridden for some condition?

[dweindl]

From the list of potentially required parameters for the parameters table, it removes those that are overridden in all conditions (mind the ~) (the ones that are overridden only in some conditions do remain). I don't think it matters whether it's always overridden or just sometimes. A parameter overridden in the condition table is never required in the parameters table nor allowed, isn't it?

libpetab-python/petab/v2/lint.py

Lines 626 to 634 in 4be03c8

	for p in problem.condition_df.columns[
	~problem.condition_df.isnull().any()
	]:
	try:
	parameter_ids.remove(p)
	except KeyError:
	pass
	return parameter_ids

[dilpath]

Yes you're right. Is there a good reason to allow estimated parameter overrides, since the effect can be achieved with some dummy parameter? Using a dummy parameter seems cleaner to me...

[dweindl]

I am not sure I understand. The following would make absolutely make sense to me:

conditionId	someParameter
c1	estimated_parameter_c1
c2	estimated_parameter_c2

What would be your alternative solution?

[dilpath]

I thought the logic was rather: if a parameter is in the column header, but is not always overridden (either by specific values or other estimated parameters), then it could be estimated such that the non-overridden (isnull) cells take the estimated value. e.g. if your example was this

conditionId	someParameter
c1	estimated_parameter_c1
c2

then someParameter could be estimated and that is the value for c2. For c1, someParameter is always overridden by estimated_parameter_c1.

[dweindl]

Oh, thanks, I see. That would be possible in principle, but I'd find that confusing.

PEtab specs are also clear that this would not be allowed:
https://github.com/PEtab-dev/PEtab/blob/b9e141dd75798d179c17262f085ed6cef8555b3e/doc/v1/documentation_data_format.rst?plain=1#L445-L449

[dilpath]

Great, I also thought it makes sense to just exclude all column header parameters 👍 Then the old code is a bug, thanks for removing!

[dilpath]

Since I was so far effectively suggesting no condition names:
I can see that arbitrary metadata like descriptive names for a PEtab component is useful, if the ID itself isn't so interpretable. But this could exist in a separate metadata table with columns like e.g. PETAB_ENTITY_ID, PETAB_ENTITY_NAME, and perhaps even things like units, if people want to annotate. I have rarely/never used these though, but since one aim of this standard is reproducibility/interoperability, ideal there is sufficient metadata to ensure two different people interpret/use the problem similarly.

[dweindl]

Agreed that some extra metadata will often be useful. Format/location to be discussed. The yaml file would an alternative option, although less convenient if the metadata itself would be in a tabular format.