Wei lin chen/crs property input builders - #418
Conversation
…d_keys, form_block, species_block and structure_block. SO107.
…LLE result tables.
…int from crs.json.
…putBuilder.to_job().
|
I do not plan to merge this immediately. There may still be useful integration I would appreciate any feedback on the proposed API and structure when you have time. |
dormrod
left a comment
There was a problem hiding this comment.
Overall, I think the public API is reasonable and would be useful. CRSJob.input_builder(...) and the property-specific helpers seem like a good direction for scripting CRS jobs.
My concern is mainly with the implementation. I’m not super familiar with all CRS details, but this appears to add a second layer of schema/workflow metadata on top of the existing CRS JSON definitions. That may be hard to maintain unless the ownership is made very explicit, or unless there are consistency tests ensuring the Python metadata stays in sync with data/input_def/crs.json, data/kf_def/crs.json, and the CRS method metadata.
I would also suggest sacrificing some of the dynamic behaviour for more explicit, well-defined builder classes. In particular, real typed attributes/signatures would make editor autocomplete and static type checking much more useful than relying mostly on runtime validation through dynamic attributes.
| builder = CRSJob.input_builder("PURESIGMAPROFILE") | ||
| builder.nprofile = 50 | ||
| builder.sigmamax = 0.025 | ||
| builder.add_compound(CRSJob.coskf_from_database("Water.coskf")) |
There was a problem hiding this comment.
From an API perspective, it might be nice to have add_compound_from_database or similar? Would avoid having to call the static method on the CRSJob again.
e.g. builder.add_compound_from_database("Water.coskf")
Or alternatively if the compound is a string, just assume it is from the database?
There was a problem hiding this comment.
Thanks, I added this as add_*_from_adfcrs_database and also renamed
CRSJob.database() to CRSJob.adfcrs_database_path(), while keeping
CRSJob.database() as a compatibility alias. CRSJob.coskf_from_database() is also
kept as an alias for CRSJob.coskf_from_adfcrs_database().
This is meant to distinguish the bundled ADFCRS database from local SQLite
databases managed by pyCRS.COSKFDatabase, which I plan to integrate in the
future.
| >>> filename = 'path/to/my/crs/inputfile.run' | ||
| >>> my_job = CRSJob.from_inputfile(filename) | ||
| >>> my_results = my_job.run() | ||
| builder = CRSJob.input_builder("SOLUBILITY", mode="gas", temperature=298.15) |
There was a problem hiding this comment.
The input builder returns ACTIVITYCOEFInputBuilder here at runtime. The methods such as add_solvent are correctly available in an IDE/Jupyter notebook, as are the attributes like isobar in the notebook as these are exposed via __dir__.
However, in a static IDE, isobar does not autocomplete as these are not exposed as attributes on the builder class. Having the attributes set up dynamically is flexible in a way, and might reduce duplication, but it doesn't help with autocompletion until runtime. Might be worth just declaring which attributes are expected on the builder classes?
It would be nice also if the descriptions could appear on hover for the attributes like "massfraction [top_level] bool: Use mass fractions; by default, fractions are interpreted as molar fractions."
There was a problem hiding this comment.
Thanks, I addressed this in the cleanup branch:
fad175f
The first cleanup commit on crs_input_builder.py only adds explicit builders for ACTIVITYCOEF,
SOLUBILITY, and BINMIXCOEF. Common inputs are now real properties on the builder
classes/mixins, so static IDEs can see attributes such as temperature, pressure,
massfraction, densitysolvent, nfrac, and mode. Compound methods are explicit as
well.
The builder still keeps only small workflow metadata for validation and mode /
compound-role behavior, while routing to settings.input or settings.input.property
is derived from crs.json.
Let me know if this revised structure is closer to what you had in mind.
| "solubility mol_per_L_solution", | ||
| "solubility g_per_L_solution", | ||
| ) | ||
| RESULT_TABLE_COMPONENT_EXTRA_QUANTITIES = ( |
There was a problem hiding this comment.
Are all these values repeated from the JSON schema? I am a bit concerned about the maintenance overhead of keeping this in sync with the crs.json and adding new things. Definitely worth talking to Robert as you mentioned in the description, to see if scm.inputs would allow some de-duplication?
There was a problem hiding this comment.
Thanks, I started over from trunk and will try to keep the changes split into
separate commits.
First, crs_definitions is now a lazy CRS metadata helper layer:
crs-builder-review-cleanup
It now only handles schema lookup from the existing CRS JSON definitions and avoids
loading CRS JSON files at import time. The builder workflow rules will be kept
separately in crs_input_builder.py.
For get_result_table, it may be cleaner by adding a field to the kf_def json, for example:
{
"_plams_result_table": {
"scope": "component",
"group": "default"
}
}
This would hopefully allow get_result_table to derive most of the default/extra
component/mixture/LLE quantity groups from kf_def.
| return ret | ||
|
|
||
| @staticmethod | ||
| def _import_pandas(method: str, requirement: str = "this method requires the 'pandas' package") -> Any: |
There was a problem hiding this comment.
(You can import and use the @requires_optional_package("pandas") decorator, as elsewhere in plams, for this and mpl)
There was a problem hiding this comment.
Thanks for the suggestion. I switched this to use the existing
requires_optional_package decorator:
|
|
||
| if TYPE_CHECKING: | ||
| import pandas as pd | ||
| from matplotlib.figure import Figure | ||
|
|
||
| __all__ = ["CRSResults", "CRSJob"] | ||
|
|
||
| __all__ = [ |
There was a problem hiding this comment.
This is a very large file. Any chance we could split it into different responsibilities (builder, plotting, results etc.)?
| RESULT_TABLE_QUANTITY_METADATA_OVERRIDES, | ||
| ) | ||
|
|
||
| _crs_json_path = Path(os.environ["AMSBIN"]) / "../data/input_def/crs.json" |
There was a problem hiding this comment.
AMSBIN is used in a couple of places here at import time. This might cause failure on import in environments without AMS
There was a problem hiding this comment.
Thanks, fixed here:
75762df
The CRS JSON path lookup was moved to crs_definitions.py and is now lazy, so it
does not require AMSBIN during import.
|
|
||
|
|
||
| # validation helpers | ||
| def _validate_required_inputs(self) -> None: |
There was a problem hiding this comment.
It might be nice to have some independent unit tests covering some of this behaviour.
Note the following example:
from scm.plams import CRSJob
builder = CRSJob.input_builder("ACTIVITYCOEF", temperature=298.15)
builder.add_solvent("../../../examples/scripting/plams_crs/Water.coskf")
builder.add_solute("../../../examples/scripting/plams_crs/Ethanol.coskf")
settings = builder.to_settings()
print(settings.input)
print(builder._metadata()["required_keys"])
the frac1 key is marked as required, but this still passes validation?
Summary
This PR adds a new CRS input-builder workflow and table-based result analysis
helpers to PLAMS.
The new
CRSJob.input_builder()API returns a builder tailored to the selectedCRS property type. It provides IDE completion and earlier runtime feedback for
property-dependent CRS inputs such as methods, modes, input keys, and compound
roles.
The PR also introduces
CRSResults.get_result_table()for converting CRS resultsto pandas tables, together with plotting helpers for sigma-profile and LLE-style
results.
Changes
before running CRS jobs.
CRSResults.get_result_table()andget_result_table_metadata().CRSResults.combine_result_tables()for combining tables from multipleCRS jobs.
diagrams.
crs.rstwith builder, table, and plotting examples.See
crs.rstfor usage examples.