Cantera / enhancements

Repository for proposed and ongoing enhancements to Cantera
11 stars 5 forks source link

Make use of 'mole' and 'molar' consistent #141

Closed ischoegl closed 2 years ago

ischoegl commented 2 years ago

Problem description

While ThermoPhase.basis has the option "mass" or "molar", Cantera uses "mass" and "mole" in other contexts. Specifically, the basis argument to

all use "mole".

It is likely easiest / least intrusive to consistently adopt "mole".

Other thoughts

The current implementation for thermo_basis using a cdef'd ThermoBasisType is hard to follow - a simple boolean flag would likely suffice.

bryanwweber commented 2 years ago

It is likely easiest / least intrusive to consistently adopt \"mole\".

On the other hand, molar is grammatically correct, if I understand correctly ☺️

ischoegl commented 2 years ago

On the other hand, molar is grammatically correct, if I understand correctly ☺️

Certainly true if used as an adjective (as in the title of the issue 😄), but I see this more as ‘unit mole’ vs ‘unit mass’ proposition. I honestly don’t have a strong preference. Imho, going to booleans may be an alternative, but the naming question remains.

speth commented 2 years ago

I agree that it would be nice to be consistent with this, but I don't really see "molar" being a better parallel to "mass" than "mole" in most of these cases. Also, there are certainly a lot more cases in Cantera where we use "mole" when this distinction needs to be made besides just the Python ThermoPhase class. The most common must be "mass fractions" and "mole fractions", but there's also cp_mass/cp_mole and related properties.

So my inclination would be standardize on "mole". (Sorry, @ischoegl, I know you already put together the PR the other way around).

bryanwweber commented 2 years ago

I expressed a preference on the issue for "molar" as the grammatically correct construction, as in "molar basis" .

ischoegl commented 2 years ago

Among the two arguments, I believe mole being used consistently is imho more convincing. I don’t think that enthalpy_molar et al. is where this should be going.

bryanwweber commented 2 years ago

I don’t think that enthalpy_molar et al. is where this should be going.

I read the underscore as "per", so enthalpy_mole is "enthalpy per mole" and wouldn't need to be changed. If it was a prefix, then "molar" would be correct and the underscore wouldn't be "per" any longer, molar_enthalpy.

Anyhow, this isn't something I feel very strongly about. Perhaps for the basis property we can retain molar as an option (that is, allow both mole and molar), but demonstrate the use of mole everywhere?

ischoegl commented 2 years ago

FWIW, the following 'overview' is somewhat interesting: https://github.com/Cantera/cantera/blob/fc6a018e00842560d64100de171d55fe9ef741e1/interfaces/cython/cantera/composite.py#L464-L508

So there isn't really a consistent way of naming beyond, especially with the partial_molar_* properties. Although not even all species-related properties follow this convention: 'mix_diff_coeffs_mole' vs. 'partial_molar_enthalpies'. Notably, property vectors related to species use plural whereas those related to reactions are singular ... :sob:

Overall, this is getting pretty nitpicky and would be more invasive than I originally realized, so I'm not so sure about this at this point. Probably this is something that should be considered when jumping from 2.6 to 3.0 after all.

PS: one option would be to raise a FutureWarning or PendingDeprecationWarning.

ischoegl commented 2 years ago

I ended up moving this over to enhancements, as this may be slightly premature for Cantera 2.6.

One other reference worth making is that some of the inconsistencies pointed out above should probably be fixed while working out a consistent naming approach for Python API and MATLAB API in Cantera/cantera#1182 (as an aside: camelCase is not something I necessarily feel strongly about).

ischoegl commented 2 years ago

Taking care of all inconsistencies would presumably break user code. Closing to avoid unnecessary churn.