Repository navigation
Vs_Profile() input checks: an empty array raises IndexError, inf is reported as NaN, 2-column units are silently ignored, and one error branch is unreachable #107
Description
Found while reviewing #74. Pre-existing on main (b764057); #74 does not change the behavior.
Problem
Four small gaps in the input checks of Vs_Profile() and check_Vs_profile_format():
- An empty array raises
IndexError.check_Vs_profile_format()never checks that there is at least one layer. For a(0, 2)array,thk[-1]raisesIndexError: index -1 is out of bounds, not aValueErrorabout the input.
PySeismoSoil/PySeismoSoil/helper_generic.py
Lines 509 to 518 in b764057
thk = data[:, 0] Vs = data[:, 1] if np.any(thk[:-1] <= 0): raise ValueError( 'The thickness column should be all positive, except for the' ' last layer.', ) if np.any(thk[-1] < 0): raise ValueError('The last layer thickness should be non-negative.') - An infinite value is reported as NaN.
check_numbers_valid()usesnp.isfinite(), so it flags both NaN and inf, but the message says "should contain no NaN values".check_two_column_format(), which many functions use, has the same message.
PySeismoSoil/PySeismoSoil/helper_generic.py
Lines 493 to 494 in b764057
if check_status == CHECK_STATUS_NOT_FINITE: raise ValueError('`data` should contain no NaN values.')
PySeismoSoil/PySeismoSoil/helper_generic.py
Lines 460 to 461 in b764057
if check_status == CHECK_STATUS_NOT_FINITE: raise ValueError(f'{name} should contain no NaN values.') - For a 2-column profile,
damping_unitanddensity_unitare accepted, stored and never used. The damping and density come fromget_xi_rho()in SI units. The two arguments are still validated and stored asprof.damping_unitandprof.density_unit, soprof.density_unitcan say'g/cm^3'whileprof.vs_profile[:, 3]is in kg/m^3. Nothing else in the package reads these attributes. The docstring describes the parameters as "The unit for the damping ratio" and "The unit for the mass density of soils", and the attributes as "Same meaning as the input parameter". It does not say that they apply only to 5-column data. This may be intended; if so, the docstrings should say so. (For 5-column data, the unit conversion has a separate problem, see Vs_Profile() converts damping and density units inside the caller's array, and its private arrays are views of that array #95.)
PySeismoSoil/PySeismoSoil/class_Vs_profile.py
Lines 56 to 60 in b764057
damping_unit : Literal['1', '%'], default='1' The unit for the damping ratio. density_unit : DensityUnit, default='kg/m^3' The unit for the mass density of soils. It can be 'kg/m^3', 'g/cm^3', 'kg/m3', or 'g/cm3'.
PySeismoSoil/PySeismoSoil/class_Vs_profile.py
Lines 86 to 89 in b764057
damping_unit : str Same meaning as the input parameter. density_unit : str Same meaning as the input parameter.
PySeismoSoil/PySeismoSoil/class_Vs_profile.py
Lines 142 to 143 in b764057
if n_col == hlp.NUM_COLUMNS_THICKNESS_AND_VS: xi, rho = sr.get_xi_rho(vs, formula_type=xi_rho_formula)
PySeismoSoil/PySeismoSoil/class_Vs_profile.py
Lines 212 to 213 in b764057
self.damping_unit = damping_unit self.density_unit = density_unit - The class's own column-count error can never be raised.
Vs_Profile.__init__ends itsif n_col == 2 ... elif n_col == 5chain with anelsethat raises "should have two or five columns". Butcheck_Vs_profile_format()already rejects any other column count, earlier, with its own message.
PySeismoSoil/PySeismoSoil/class_Vs_profile.py
Lines 191 to 195 in b764057
else: raise ValueError( 'The dimension of the input data is wrong. It ' 'should have two or five columns.', )
PySeismoSoil/PySeismoSoil/helper_generic.py
Lines 499 to 507 in b764057
if data.shape[1] not in { NUM_COLUMNS_THICKNESS_AND_VS, NUM_COLUMNS_OF_FULL_VS_PROFILE, }: raise ValueError( '`data` should have either ' f'{NUM_COLUMNS_THICKNESS_AND_VS} or ' f'{NUM_COLUMNS_OF_FULL_VS_PROFILE} columns.' )
import numpy as np
import PySeismoSoil.helper_generic as hlp
from PySeismoSoil.class_Vs_profile import Vs_Profile
# 1. Empty profile
try:
Vs_Profile(np.empty((0, 2)))
except Exception as e:
print(type(e).__name__, e)
# IndexError index -1 is out of bounds for axis 0 with size 0
# (expected: a ValueError about the empty profile)
# 2. Infinite value
try:
Vs_Profile(np.array([[10.0, np.inf], [0.0, 500.0]]))
except Exception as e:
print(type(e).__name__, e)
# ValueError `data` should contain no NaN values.
# (expected: a message that also names infinite values)
try:
hlp.check_two_column_format(np.array([[0.0, 1.0], [0.01, np.inf]]))
except Exception as e:
print(type(e).__name__, e)
# ValueError `something` should contain no NaN values.
# 3. Units for a 2-column profile
prof = Vs_Profile(np.array([[10.0, 0.0], [200.0, 500.0]]).T, damping_unit='%', density_unit='g/cm^3')
print(prof.damping_unit, prof.density_unit)
# % g/cm^3
print(prof.vs_profile[:, 2], prof.vs_profile[:, 3])
# [0.04166667 0.01666667] [1800. 1800.] (damping in 1, density in kg/m^3)
# 4. The class's own column-count error is never raised
try:
Vs_Profile(np.ones((3, 4)))
except Exception as e:
print(type(e).__name__, e)
# ValueError `data` should have either 2 or 5 columns. (from check_Vs_profile_format())Fix
- In
check_Vs_profile_format(), after the column check, raiseValueError('datashould have at least one layer.')whendata.shape[0] == 0. - In
check_Vs_profile_format()andcheck_two_column_format(), change the message to "should contain no NaN or infinite values". Update thematchof the NaN case of the Vs_Profile format test (test_Vs_profile_format__case_3onmain,test_Vs_profile_format[NaN_values]in Combine near-duplicate test functions into parametrized tests #74), which matches 'should contain no NaN values'. - In the
Vs_Profiledocstring, say thatdamping_unitanddensity_unitapply only to 5-column data, and thatvs_profilealways uses damping in 1 and density in kg/m^3. - In
Vs_Profile.__init__, changeelif n_col == hlp.NUM_COLUMNS_OF_FULL_VS_PROFILE:toelse:and remove the unreachableelse: raise ValueError(...)branch. - Add cases to
test_Vs_profile_formatfor an empty(0, 2)array and for an array withnp.inf.
With steps 1, 2 and 4 on main (and the test match updated), the repro prints "should have at least one layer" and "should contain no NaN or infinite values" for items 1 and 2, and all 240 tests pass.
Activity