Repository navigation
Wei lin chen/crs builder review cleanup - #426
Merged
Merged
Conversation
Keep CRS schema lookup separate from builder workflow logic and avoid loading CRS JSON files at import time.
Add property-specific input builders for ACTIVITYCOEF, SOLUBILITY, and BINMIXCOEF with typed properties and explicit compound-role methods. Keep CRSJob as a thin factory wrapper, derive input-key routing from crs.json, and centralize mode, required-input, and compound-key validation in the builder module.
This distinguishes the ADFCRS database from local SQLite databases managed by pyCRS.COSKFDatabase.
Add runtime normalization and validation for explicit CRS input-builder properties, including float ranges, scalar floats, intergers and booleans.
Keep unknown COMPOUND keys as errors, but ignore schema-valid keys that do not affect the selected builder. For example, ACTIVITYCOEF ignores meltingpoint instead of rejecting it.
Use crs.json input metadata for primitive value validation, and keep _SINGLE_VALUE_INPUT_KEYS as the builder-level override for float_list inputs that are semantically single values.
Expose sigma moment input keys on sigma profile builders, refine hover descriptions for explicit builder properties, and add a separate expert-option API for technical top-level CRS input keys.
dormrod
reviewed
Sep 22, 2026
dormrod
reviewed
Sep 22, 2026
…y CRSJob (was not determined automatically by pyright at least) SO--
robertrueger
approved these changes
Sep 22, 2026
robertrueger
left a comment
robertrueger
left a comment
Member
There was a problem hiding this comment.
The user interface looks pretty nice. I think the implementation would have been much more straightforward if we had the new new builder classes based on pydantic, as that would do all the validation based on the type-hints automatically. But that alone is probably not a good reason to pull in pydantic as a new dependency of PLAMS ...
Contributor
Author
…eter_set_to_settings and apply_parameter_set_to_inputs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For instructions on using the property-aware CRS input builder, see the
https://github.com/SCM-NV/PLAMS/blob/WeiLinChen/crs-builder-review-cleanup/doc/source/interfaces/crs.rst
I’m not sure whether class CRSInputBuilder.to_inputs() is the structure we want, but it provides a convenient way to convert the builder state into a typed scm.inputs.CRS model. This also allows users to configure additional CRS options that are not exposed by the property-specific builder.