Skip to content

Add a 'partial' model type#115

Merged
rtimms merged 4 commits into
FaradayInstitution:mainfrom
pipliggins:40-validation
Oct 31, 2025
Merged

Add a 'partial' model type#115
rtimms merged 4 commits into
FaradayInstitution:mainfrom
pipliggins:40-validation

Conversation

@pipliggins
Copy link
Copy Markdown
Collaborator

@pipliggins pipliggins commented Sep 29, 2025

Fixes #40

Adds 'Partial' as a model option, which rules that all the top-level parameters (cell, electrodes, separator, electrolyte, user-defined) are optional, along with the 'State' section. Any parameter group (e.g. Cell) provided must be complete.

Checks that:

  1. If both electrodes are specified, they are of the same type (i.e. both SPM or both full)
  2. If one or more electrodes are SPM, then 'full' parameters (electrolyte & separator) are not provided.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 29, 2025

Codecov Report

❌ Patch coverage is 98.21429% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@22d8f3b). Learn more about missing BASE report.

Files with missing lines Patch % Lines
bpx/schema.py 98.21% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #115   +/-   ##
=======================================
  Coverage        ?   96.73%           
=======================================
  Files           ?        9           
  Lines           ?      460           
  Branches        ?       47           
=======================================
  Hits            ?      445           
  Misses          ?        7           
  Partials        ?        8           

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

Copy link
Copy Markdown
Collaborator

@martinjrobins martinjrobins left a comment

Choose a reason for hiding this comment

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

code looks good to me, thanks @pipliggins. I'll let @rtimms check the match with the published schema

Copy link
Copy Markdown
Collaborator

@rtimms rtimms left a comment

Choose a reason for hiding this comment

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

thanks @pipliggins LGTM! Could you update the changelog, please?

@pipliggins
Copy link
Copy Markdown
Collaborator Author

thanks @pipliggins LGTM! Could you update the changelog, please?

Done @rtimms - If a partial schema is defined, should the State section introduced in #113 then be optional? If so it might be easier to merge that one first, then I can make the required changes here before merging this one.

@pipliggins
Copy link
Copy Markdown
Collaborator Author

thanks @pipliggins LGTM! Could you update the changelog, please?

Done @rtimms - If a partial schema is defined, should the State section introduced in #113 then be optional? If so it might be easier to merge that one first, then I can make the required changes here before merging this one.

The 'State' section introduced by #113 is now optional for partial models, with a custom check to ensure it's present for anything other than model='partial'

@rtimms
Copy link
Copy Markdown
Collaborator

rtimms commented Oct 31, 2025

@pipliggins great, thanks!

@rtimms rtimms merged commit 3b068a5 into FaradayInstitution:main Oct 31, 2025
12 checks passed
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.

Separate out validity from completeness

4 participants