Conversation
There was a problem hiding this comment.
Pull request overview
Updates QCManyBody’s ManyBodyProperties serialization and cross-version conversion to match recent QCElemental v2 behavior (notably schema_name appearing in model_dump() while omitting None fields), and adjusts tests accordingly.
Changes:
- Update v2
ManyBodyPropertiesto filter outNonevalues during serialization while retainingschema_name. - Add/extend
convert_vsupport for ManyBody properties and ensure v2→v1 result conversion converts nested property models. - Update schema keyword tests to account for v2
model_dump()includingschema_name.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
qcmanybody/tests/test_schema_keywords.py |
Updates assertions to reflect v2 serialization (schema_name included) and uses exclude_unset for stable dict-size checks. |
qcmanybody/models/v2/many_body.py |
Refactors v2 properties model generation/serialization; adds ManyBodyProperties.convert_v; strengthens v2→v1 result conversion for properties. |
qcmanybody/models/v1/manybody_output_pydv1.py |
Adds ManyBodyResultProperties.convert_v and ensures v1→v2 result conversion converts properties. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| dself["specification"].pop("schema_name") | ||
| dself["specification"]["specification"]["protocols"].pop("schema_name") | ||
| except KeyError: | ||
| for spec in dself["specification"].values(): | ||
| spec.pop("schema_name") | ||
| spec["protocols"].pop("schema_name") |
There was a problem hiding this comment.
In ManyBodySpecification.convert_v, the try branch assumes dself["specification"] has a schema_name key and then indexes dself["specification"]["specification"]["protocols"], but specification is typed/validated as a dict of AtomicSpecifications, so this branch looks unreachable and the ...["specification"]["protocols"] path appears inconsistent with the structure used in the except branch (spec["protocols"]). Simplifying this to always iterate for spec in dself["specification"].values() (and dropping the nested ...["specification"]["specification"]...) would make the conversion logic clearer and avoid a latent crash if the try branch ever becomes reachable.
| try: | |
| dself["specification"].pop("schema_name") | |
| dself["specification"]["specification"]["protocols"].pop("schema_name") | |
| except KeyError: | |
| for spec in dself["specification"].values(): | |
| spec.pop("schema_name") | |
| spec["protocols"].pop("schema_name") | |
| # strip schema_name from each atomic specification and its protocols | |
| for spec in dself["specification"].values(): | |
| spec.pop("schema_name", None) | |
| if "protocols" in spec: | |
| spec["protocols"].pop("schema_name", None) |
ManyBodyProperties.model_dump()now includesschema_name, but not all the Nones