Skip to content

Conversation

@prehner
Copy link
Contributor

@prehner prehner commented Feb 13, 2025

  • Removes PyIdentifier, PyChemicalRecord and builds the Python classes directly from the rust versions instead.
  • Removes the impl_pure_record, impl_binary_record, and impl_segment_record macros in favor of the new PyPureRecord, PyBinaryRecord and PySegmentRecord that use IndexMap (kwargs) instead of generics
  • Splits BinaryRecord<I,M> into BinaryRecord<M> and BinarySegmentRecord. BinarySegmentRecord is (for now) not generic, but rather always using f64 as model record.

No changes to the Rust interface at all. Will break all models that use non-f64 parameters (i.e., bool flags, enums, Vecs (especially with non-fixed lengths)). Therefore, the next step has to be to unify the parameter creation which I think is worth it in terms of maintainability and structure of the Python code.

@g-bauer
Copy link
Contributor

g-bauer commented Mar 18, 2025

Several model's parameter records have fields that are non-f64, but as long as they are de/serializable (which they all are), we can use pythonize and/or serde_json to have a single generic PyXRecord (X = Binary, Pure, Segment, ...). I see two options:

Use Py<PyDict>

A record can use Py<PyDict> as generic storage:

#[pyclass]
struct PyPureRecord {
    identifier: Identifier,
    molarweight: f64,
    model_record: Py<PyDict>
}

impl<M> TryInto<PureRecord<M>> for PyPureRecord
where
    for<'de> M: Deserialize<'de>,
{
    type Error = ParameterError;
    fn try_into(self) -> Result< PureRecord<M>, ParameterError> {
        let mr: Result<M, Self::Error> = Python::with_gil(|py| {
            depythonize(&self.model_record.bind(py))
                .map_err(|e| ParameterError::IncompatibleParameters(e.to_string()))
        });
        Ok(PureRecord::new(self.identifier, self.molarweight, mr?))
    }
}
  • Advantage:
    • It is easy to work with, since the Python constructors take kwargs as &Bound<'_, PyDict>.
    • We can directly use setter/getter for the model record field.
  • Disadvantage:
    • We need to integrate it with the garbage collector (__clear__, __traverse__) since we store a Python pointer.
    • Generically parsing from json (e.g. in the macros) is not so straightforward (we could wrap Py<PyDict> into a ser/de new type struct).

Use serde_json::Value

We can serialize a Py<PyDict> into a serde_json::Value via depythonize and store that.

#[pyclass]
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct PyPureRecord {
    identifier: Identifier,
    molarweight: f64,
    model_record: serde_json::Value,
}

impl<M> TryInto<PureRecord<M>> for PyPureRecord
where
    for<'de> M: Deserialize<'de>,
{
    type Error = ParameterError;
    fn try_into(self) -> Result<PureRecord<M>, ParameterError> {
        let mr = serde_json::from_value(self.model_record);
        Ok(PureRecord::new(self.identifier, self.molarweight, mr?))
        // or: Ok(serde_json::from_value(serde_json::to_value(self)?)?)
    }
}

#[pymethods]
impl PyPureRecord {
    #[new]
    #[pyo3(signature = (identifier, molarweight, **parameters))]
    fn new(
        identifier: Identifier,
        molarweight: f64,
        parameters: Option<&Bound<'_, PyDict>>,
    ) -> Result<Self, ParameterError> {
        let mr: serde_json::Value = depythonize(parameters.unwrap()).map_err(...)?;
        // ...
  • Advantage:
    • Python record is de/serializable.
  • Disadvantage:
    • Setter/getter have to de/pythonize every time (but performance of building rarely matters)

General problems

  • The actual arguments/fields for building a model record are only checked when they are used to build a specific parameter instance, e.g. via from_records like PcSaftParameters.from_records(...).
  • Additional parameters are ignored by serde. This can be nasty when one has a typo in an optional parameter and it is silently ignored unless we use serde's deny_unknown_fields (which does not work with flatten fields). Consider pr = PureRecord(..., sigma=3.0, epsilon_k=150.0, m=1.0, mue=2.0) which will ignore the mue silently.

@g-bauer
Copy link
Contributor

g-bauer commented Mar 18, 2025

I implemented the version with serde_json::Value. Repeated some code in favor of another macro and multiple pymethod impl blocks.

Todo

  • binary_records getter (unwrapping on f64::try_from crashes some models) - maybe two methods are better: one getter and one that tries to convert to single float.
  • add docstrings
  • clean up rest of uncommented parameter code

@prehner prehner mentioned this pull request Apr 5, 2025
@prehner
Copy link
Contributor Author

prehner commented Apr 5, 2025

I introduced PyParameters and PyGcParameters that basically hold pure records and binary records together. With those, we can also get rid of the impl_parameters macro. The calls in Python now look like this:

from feos.eos import EquationOfState
from feos import PureRecord, Identifier, Parameters

pr = PureRecord(Identifier(), 12.0, m=1.2, sigma=3.0, epsilon_k=150, mu=2.4)
par = Parameters.from_records([pr])
eos = EquationOfState.pcsaft(par)

or, e.g.,

par = GcParameters.from_json_smiles(["CCCCCC"], "sauer2014_smarts.json", "sauer2014_hetero.json")
eos = EquationOfState.gc_pcsaft(par)
State.critical_point(eos)

Errors in the construction of the parameters will now only be recognized at the point of instantiating the eos, the Parameter class merely stores records, does the JSON handling, and the bookkeeping for mixtures.

There is some unpleasant redundancy in the PyParameters struct and the Parameters trait that we might get rid off at some point by adjusting the interfaces on the rust side. For now, I can live with that. Also, we lose some quality in the output and accessibility of parameters. This is unfortunate, but I think we are better off implementing a more generic solution once instead of copy-pasting long functions that generate markdown representations of parameters (which are kind of unmaintained at the moment anyways).

Todos

  • adjust the non-PC-SAFT models
  • clean up docstrings, and improve representations (for the future)

@prehner prehner merged commit 4bfa9f8 into main Apr 7, 2025
15 checks passed
@prehner prehner deleted the pure_record branch April 7, 2025 15:46
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.

3 participants