New validation API

[dweindl]

For petab.v2, refactor the current validation setup to

• be more modular
• allow PEtab extensions to register additional validation steps in the future
• make validation messages accessible programmatically (previously, they were logged directly)

Also:

• Make petablint work with both v1 and v2 problems.
• Add first validation rules that are specific to v2

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 44.08602% with 156 lines in your changes missing coverage. Please review.

Project coverage is 72.92%. Comparing base (6fff855) to head (3ad773d).

Files	Patch %	Lines
petab/v2/lint.py	57.21%	63 Missing and 20 partials ⚠️
petab/petablint.py	0.00%	38 Missing ⚠️
petab/versions.py	0.00%	20 Missing ⚠️
petab/v2/problem.py	44.44%	12 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #288      +/-   ##
===========================================
- Coverage    74.37%   72.92%   -1.45%     
===========================================
  Files           70       72       +2     
  Lines         4460     4717     +257     
  Branches       953     1012      +59     
===========================================
+ Hits          3317     3440     +123     
- Misses         872      983     +111     
- Partials       271      294      +23     

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

[dilpath]

Looks great 👍

[dilpath]

Any opinion whether this should import from petab instead of petab.v1, so it's designed to fail if we're still using v1 linters by the time we release v2? Or do we expect that we continue to use v1 linters in v2 where relevant?

[dweindl]

So far, my approach was reusing what is reusable, and once something changes, we add our own implementation in .v2. So, yes, I would keep the import from v1.

[dilpath]

Why would multiple model IDs map to a single PEtab ID? Is it e.g. to observe the sum of all species regardless of isotope state in rule-based modeling? Then I would have more questions.. but irrelevant here.

[dweindl]

Iirc, the only reason is that so far, it's not explicitly forbidden.

[dilpath]

For this single-error cases, there could already be some SingleValidationTask class like we discussed, e.g.

Suggested change

	class CheckVisualizationTable(ValidationTask):
	"""A task to validate the visualization table of a PEtab problem."""
	def run(self, problem: Problem) -> ValidationIssue | None:
	if problem.visualization_df is None:
	return
	from petab.visualize.lint import validate_visualization_df
	if validate_visualization_df(problem):
	return ValidationIssue(
	level=ValidationIssueSeverity.ERROR,
	message="Visualization table is invalid.",
	)
	class CheckVisualizationTable(SingleValidationTask):
	"""A task to validate the visualization table of a PEtab problem."""
	level = ValidationIssueSeverity.ERROR
	message = "Visualization table is invalid."
	def run(self, problem: Problem) -> ValidationIssue | None:
	if problem.visualization_df is None:
	return
	from petab.visualize.lint import validate_visualization_df
	return (not?) validate_visualization_df(problem)
	or
	return self.handle(validation_visualization_df(problem))

[dweindl]

I am not sure this will be more intuitive. People will always wonder whether true indicates passed or failed. Where the message is context-specific, this wouldn't really save anything. Will leave as is for now.

[dilpath]

Ah, I intended self.handle to return ValidationIssue or None like done elsewhere in this PR

[dilpath]

Where the message is context-specific

Are you saying, an error message for SingleValidationTasks might be context-specific? Doesn't that contradict the idea of a SingleValidationTask?

[dweindl]

I meant that instead of There are log-scaled parameters with negative bounds., we want to say There are log-scaled parameters with negative bounds: parameter1, parameter42., so one would have to modify self.message again somewhere, and wouldn't really gain much.

[dilpath]

I see, but not if self.handle also accepts an optional level/message. Anyway, not important