Skip to content

Conversation

dweindl
Copy link
Member

@dweindl dweindl commented Sep 24, 2025

  • Update schema
  • v2.Problem.id
  • v2.ProblemConfig.id

Related to PEtab-dev/PEtab#646.
Closes #441.

* Update schema
* v2.Problem.id
* v2.ProblemConfig.id

Related to PEtab-dev/PEtab#646.
Closes PEtab-dev#441.
@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2025

Codecov Report

❌ Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.80%. Comparing base (f79bba5) to head (511c6d4).

Files with missing lines Patch % Lines
petab/v2/core.py 63.63% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #442      +/-   ##
==========================================
- Coverage   74.82%   74.80%   -0.02%     
==========================================
  Files          62       62              
  Lines        6792     6803      +11     
  Branches     1203     1205       +2     
==========================================
+ Hits         5082     5089       +7     
- Misses       1255     1258       +3     
- Partials      455      456       +1     

☔ 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.

@dweindl dweindl marked this pull request as ready for review September 29, 2025 13:23
@dweindl dweindl requested a review from a team as a code owner September 29, 2025 13:23
Copy link
Member

@dilpath dilpath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

petab/v2/core.py Outdated
Comment on lines 1633 to 1636
@property
def id(self) -> str:
"""The ID of the PEtab problem if set, ``None`` otherwise."""
return self.config.id if self.config else None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@property
def id(self) -> str:
"""The ID of the PEtab problem if set, ``None`` otherwise."""
return self.config.id if self.config else None
@property
def id(self) -> str | None:
"""The ID of the PEtab problem if set, ``None`` otherwise."""
return self.config.id if self.config else None

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, I was undecided whether we should go for None or '' if no ID is set. I think always having a string could be more convenient. Any preference?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think the Pythonic way would be to use None, because it's the absence of an ID. Also the "no ID" case is a special case that will always need to be handled, e.g. to avoid writing a file name .yaml to disc. Maybe using None then makes it easier to detect errors since the special case is more explicit. No strong opinion though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memo to self: The yaml schema may need updating. If we distinguish empty strings and null/None, the former should raise a validation error whereas the latter should not.

@dweindl dweindl merged commit 004be14 into PEtab-dev:main Oct 2, 2025
17 of 19 checks passed
@dweindl dweindl deleted the v2_id branch October 2, 2025 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add id field to v2.ProblemConfig

3 participants