Update/use ProblemConfig #353
Conversation
dweindl
commented
Dec 20, 2024
dweindl
commented
- add to_yaml
- refactor model, avoid extra elements in object tree
- use ProblemConfig in petab1to2
* add to_yaml * refactor model, avoid extra elements in object tree * use ProblemConfig in petab1to2
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #353 +/- ##
===========================================
- Coverage 74.89% 74.83% -0.06%
===========================================
Files 56 56
Lines 5604 5600 -4
Branches 982 982
===========================================
- Hits 4197 4191 -6
- Misses 1033 1035 +2
Partials 374 374 ☔ View full report in Codecov by Sentry. |
| condition_files: ListOfFiles = [] | ||
| observable_files: ListOfFiles = [] | ||
| visualization_files: ListOfFiles = [] | ||
| sbml_files: list[str | AnyUrl] = [] |
There was a problem hiding this comment.
This can be list[Path | AnyUrl], since pydantic has native support for pathlib.Path IIRC. But then you might need to make a custom path object that serializes to string https://github.com/dilpath/mkstd/blob/fb4ebfda629b0fb9eedb1def5ab05438ab85696e/mkstd/types/path.py
Fine as is, just noting in case it becomes useful later
petab/v1/problem.py
Outdated
| filename: Destination file name. The parent directory will be created | ||
| if necessary. |
There was a problem hiding this comment.
Not in __all__ so I guess it doesn't matter for docs, but usually this style in this repo
| filename: Destination file name. The parent directory will be created | |
| if necessary. | |
| Parameters: | |
| filename: | |
| Destination file name. The parent directory will be | |
| created if necessary. |
.. I think. In the files touched by this PR alone, I see
Parameters
----------
Arguments:
Parameters:
There was a problem hiding this comment.
Yeah, it would make sense to start a style guide....
I'd prefer :param ...:
There was a problem hiding this comment.
Agreed, but no preference for the specific style
| @@ -1024,7 +1023,17 @@ class ProblemConfig(BaseModel): | |||
| description="The base path to resolve relative paths.", | |||
| exclude=True, | |||
| ) | |||
| format_version: VersionNumber = "2.0.0" | |||
| format_version: str = "2.0.0" | |||
There was a problem hiding this comment.
str | int in v1 -- intentional difference?
There was a problem hiding this comment.
That's how it is in the current schema draft. Although not strictly enforced so far, I think it would make sense to always require major.minor. But that's probably a separate discussion.
There was a problem hiding this comment.
Since there is no benefit to keeping 1 as an int, could use str instead of str | int, to use pydantic to handle the type casting.
Requiring a full version string is fine for me, but then we need to provide schemas for each minor version. Makes sense to support minor updates to the format, so I'm also in favor of expecting a full X.X or X.X.X string. major.minor` as suggested is probably enough since we probably don't need to distinguish "minor" and "patch" updates
There was a problem hiding this comment.
Agreed, but I'll keep that for a future update.
petab/v2/problem.py
Outdated
| def to_yaml(self, filename: str | Path): | ||
| """Write the configuration to a YAML file. | ||
|
|
||
| filename: Destination file name. The parent directory will be created |
There was a problem hiding this comment.
Same doc comment re: Parameters:
