[Review] Default unit converters for time are incorrect in subsecond range

[fwitte]

Following the logic of the length converters https://github.com/chmarti1/PYroMat/blob/d65cb2c73bd14b756deca4b7271246322515d221/src/pyromat/units.py#L277-L281, the time converts for subsecond time should be with a negative power, or?

https://github.com/chmarti1/PYroMat/blob/d65cb2c73bd14b756deca4b7271246322515d221/src/pyromat/units.py#L292-L294

I unfortunately did not yet find a way to test it myself, that's why I opened the issue.

[fwitte]

So, I cannot find where the "default" time conversion table is used. But calling something like time.__call__(10, "s", "ms") after the variable definition in the setup() function does indeed return 0.01 instead of the expected 10000.

[fwitte]

Furthermore, the example conversion inch to feet is not correct:

https://github.com/chmarti1/PYroMat/blob/d65cb2c73bd14b756deca4b7271246322515d221/src/pyromat/units.py#L92-L94

The result here is 72 not 0.5, the data should be switched in the input dictionary or the result must be changed.

Usually, I like to also test my examples in the docstrings with pytest. You can do that with --doctest-modules, which collects all docstring examples and tests them. I tried to run that with the pyromat.units module, but there seem to be some issues with the setup() method. I am getting

>       const_dstd = const_pstd*1e5/const_Ru/const_Tstd  # mol/m^3
E       TypeError: unsupported operand type(s) for /: 'float' and 'module'

on python -m pytest --doctest-modules ./src/pyromat/units.py.

Apparently, const_Tstd is a module (?!), which really does not make sense to me. I could not yet figure out, how to solve that. So there might be some pytest related issues, that need to be sorted out before implementing doctesting.

[jranalli]

Thanks for these! I wrote a unit testing script to validate these as the fix was implemented, and identified a few other bugs as well.

@chmarti1 I'll create a separate branch w/pull request when finished, for you to review.

[chmarti1]

Thanks for your diligence!

1. time units: You are correct - this behavior is wrong, and it should be corrected. The good news is that it does not affect the package's functionality - none of the properties use time. It was merely added for completeness. The conversion values are set on line 291 inside the setup() function. They are commented as "validated" in 2017. I could trace my records to see why they are not correct, but it is easier to just fix them.

2. length units: I don't see an issue with length conversion. Here are the results I get, which are correct.

   >>> pm.units.length(6, from_units='in', to_units='ft')
   0.5
   >>> pm.units.length(1,from_units='m', to_units='in')
   39.37007874015748

3. Tstd The setup() function assigns values to all of the constants (including const_Tstd) and then uses those to construct the unit conversion tables. It appears you are encountering an error on line 263, shortly after const_Tstd is assigned on line 259. The Tstd argument appears first in the setup() call signature, so if the setup function were being called as if it were a bound method (instead of a function), that might cause the units module to be passed to Tstd instead of a number.

When I load PYroMat normally,

>>> import pyromat as pm
>>> pm.units.const_Tstd
273.15
>>> pm.units.const_dstd
44.61503340547032
>>> pm.units.setup(Tstd=298.15)
>>> pm.units.const_Tstd
298.15
>>> pm.units.const_dstd
40.87404452357611

[jranalli]

Furthermore, the example conversion inch to feet is not correct:

https://github.com/chmarti1/PYroMat/blob/d65cb2c73bd14b756deca4b7271246322515d221/src/pyromat/units.py#L92-L94

Oh I see what the concern is here. It's that the example dict listed is incorrect.

The definition is:

conv = self.table[from_units] / self.table[to_units]

So if from_units are inches and to_units are ft, we need in = 1/12 and ft = 1 in the table. It's actually an inverse of how the documentation describes it. Added a commit to the pull request #63 clearing up the documentation.

[chmarti1]

Confirmed. It looks like the in-line documentation has it backwards. See line 147 in units.py, the conversion factor is calculated as

https://github.com/chmarti1/PYroMat/blob/d65cb2c73bd14b756deca4b7271246322515d221/src/pyromat/units.py#L147

I seem to remember making a decision to reverse this behavior back when the units module was under development. It looks like the documentation was not correctly updated. Good catch.

[fwitte]

@jranalli, @chmarti1: Finishing the review right now, thank you for your prompt replies and please forgive my late answer here :).