fix: young's modulus unit

[anyangml]

Summary

Major changes:

fix Young's Modulus Unit, resolve #4435

Checklist

• Google format doc strings added. Check with ruff.
• Type annotations included. Check with mypy.
• Tests added for new features/fixes.
• If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

[anyangml]

@shyuep Please check whether the formula and units used in calculating Clarke's thermal conductivity are correct. According to GPT, a Reduced Planck constant is missing.

[anyangml]

following up on this

[shyuep]

@mkhorton @esoteric-ephemera Pls review this short PR. Do we want to really return the GPa elastic tensor or do we just update the doc to note that it is in eV/A3? I think this will cause all hell to break loose in downstream stuff like MP?

[esoteric-ephemera]

I checked and the only dimensional elasticity data we have on MP are the bulk and shear moduli, which get corrected converted (slightly outdated builder link) to GPa from kilobar. The derived data that would be inconsistent with the docstr is visible when you pull elastic data from the API or AWS

My vote is for returning consistent units without conversion and let the user decide which system of units to use

[esoteric-ephemera]

@anyangml I would change the docstr to:

Calculates Young's modulus using Voigt-Reuss-Hill averages of bulk and shear moduli.

and remove the conversion to GPa here (and elsewhere where SI units are reported):

return 9* self.k_vrh * self.g_vrh / (3 * self.k_vrh + self.g_vrh)

[esoteric-ephemera]

Also @anyangml on the question of other missing units in the thermal conductivity, here's a good reference which summarizes the Clark, Cahill, and Agne thermal conductivity methods:

• General form: $\kappa \approx c \rho^{2/3} k_B (2 v_T + v_L) / 3$
• Cahill: $c = 1.21$
• Clark: $c=0.93 $
• Agne et al.: $c = 0.76$

These should probably get unified into an underlying method:

def _estimate_thermalcond(self, structure: Structure, cons: float) -> float:
        """Estimate thermal conductivity using semi-classical methods.

        Args:
            structure: pymatgen structure object
            cons : float

        Returns:
            float: thermal conductivity
        """
        n_sites = len(structure)
        site_density = n_sites / structure.volume
        return cons * kB* site_density ** (2 / 3) * (self.long_v(structure) + 2 * self.trans_v(structure)) / 3

Tagging @naik-aakash who implemented these first to make sure I got that right

[naik-aakash]

Also @anyangml on the question of other missing units in the thermal conductivity, here's a good reference which summarizes the Clark, Cahill, and Agne thermal conductivity methods:

• General form:
  κ ≈ cρ2 /3kB(2vT+vL)/3

• Cahill: c = 1.21

• Clark: c = 0.93

• Agne et al.: c = 0.76

These should probably get unified into an underlying method:

def _estimate_thermalcond(self, structure: Structure, cons: float) -> float:
        """Estimate thermal conductivity using semi-classical methods.

        Args:
            structure: pymatgen structure object
            cons : float

        Returns:
            float: thermal conductivity
        """
        n_sites = len(structure)
        site_density = n_sites / structure.volume
        return cons * site_density ** (2 / 3) * (self.long_v(structure) + 2 * self.trans_v(structure)) / 3

Tagging @naik-aakash who implemented these first to make sure I got that right

Hi @esoteric-ephemera , I had added only for the Agne model. Currently the formula seems fine to me, but Boltzmann constant (kB) seems to be missing in the function proposed, and site_density needs to be in m−3 (this conversion is also not there). So the final value will not be in (W/(m·K)).

I would have expected returned values to be in (W/(m·K)). I'm not sure what the planned change is here.

[esoteric-ephemera]

Thanks for the context and typo fix @naik-aakash - updated the formula above. I think we're trying to avoid coercing values into SI units since there's no easy way to check the units input by a user (without adding a lot of pint-based infrastructure)

So the units won't be in W/(m . K) unless the input units are also SI

[JaGeo]

@esoteric-ephemera i would be concerned about lots of breaking changes, especially for the units.
Users will not be able to easily discover this. Or do I get the discussion wrong?

[esoteric-ephemera]

I also don't like the idea of breaking changes, but I think we need to at least alert users about potential incorrect unit conversion from this module. What about:

1. Keep the current functions but throw a warning to the user about incorrect unit conversion/deprecation, add a deprecated_ prefix to them
2. Add new functions which do not perform unit conversion and use the current names of the functions. Throw a warning about changes to the unit conversion

Ex:

@property
    def deprecated_y_mod(self) -> float:
        """
        Calculates Young's modulus (in non-standard units) using the
        Voigt-Reuss-Hill averages of bulk and shear moduli.
        """
        warnings.warn("This function which incorrectly performs unit conversion is deprecated and slated for removal on 2026-01-01, but is currently maintained for backwards compatibility")
        return 9.0e9 * self.k_vrh * self.g_vrh / (3 * self.k_vrh + self.g_vrh)

@property
    def y_mod(self) -> float:
        """
        Calculates Young's modulus using the
        Voigt-Reuss-Hill averages of bulk and shear moduli.
        The units are the same as those for the bulk/shear moduli.
        """
        warnings.warn("This function no longer performs unit conversion. For the older function, which performed incorrect unit conversion and will be removed, please see `deprecated_y_mod`.")
        return 9 * self.k_vrh * self.g_vrh / (3 * self.k_vrh + self.g_vrh)

[JaGeo]

@esoteric-ephemera i would prefer this approach!