Skip to content

Fix broken diameter-range validation in getLineProps#67

Merged
shousner merged 1 commit into
NatLabRockies:devfrom
gaoflow:fix/getlineprops-diameter-dev
Jun 4, 2026
Merged

Fix broken diameter-range validation in getLineProps#67
shousner merged 1 commit into
NatLabRockies:devfrom
gaoflow:fix/getlineprops-diameter-dev

Conversation

@gaoflow

@gaoflow gaoflow commented Jun 1, 2026

Copy link
Copy Markdown

Two bugs prevent getLineProps from validating the nominal diameter against a material's d_min / d_max bounds.

Bug 1 — d referenced before assignment

In getLineProps (helpers.py) the diameter-range checks use d before it is defined:

# Check valid diameter ranges
if mat['d_max'] >= 0 and d > mat['d_max']:   # d not defined yet!
    raise Exception(...)
if mat['d_min'] >= 0 and d < mat['d_min']:   # d not defined yet!
    raise Exception(...)

# calculate the relevant properties for this specific line type
d = dnommm*0.001          # <-- d is only assigned here, AFTER the checks

So for any material whose d_min >= 0 or d_max >= 0, the call raises UnboundLocalError: cannot access local variable 'd' instead of the intended out-of-range Exception. Fixed by moving the d = dnommm*0.001 conversion above the checks.

Bug 2 — d_max typo in loadLineProps

output[mat]['d_max'] = getFromDict(props, 'd_dmax', default=-1.0)  # reads the wrong key

The key 'd_dmax' never exists, so a user-specified d_max is silently dropped to the -1 "disabled" default and the upper-bound check never fires. Fixed by reading 'd_max'.

The two bugs masked each other: because d_max always fell back to -1, the d > mat['d_max'] branch of Bug 1 never executed with the default database, so the crash only surfaced via d_min.

Reproduction (before this PR)

from moorpy.helpers import getLineProps, loadLineProps

# Bug 2: user-set d_max is dropped
src = {"lineProps": {"mat": {"mass_d2": 100.0, "MBL_0": 0.0, "MBL_d": 1e6,
                             "d_min": 0.0, "d_max": 0.2}}}
print(loadLineProps(src)["mat"]["d_max"])     # -> -1.0  (should be 0.2)

# Bug 1: UnboundLocalError instead of the intended range error
src2 = {"lineProps": {"mat": {"mass_d2": 100.0, "MBL_0": 0.0, "MBL_d": 1e6,
                              "d_min": 0.05}}}
getLineProps(10.0, "mat", source=src2)        # -> UnboundLocalError: ... 'd'

After this PR

  • d_max from the source is honoured.
  • An out-of-range diameter raises the intended Exception ("… less than the min valid value …" / "… greater than the max valid value …") for both bounds.
  • In-range diameters and the default property database (no d_min/d_max set) are unaffected.

Verification

  • New regression test tests/test_line.py::test_getLineProps_diameter_limits covers the in-range success path and both the d_min and d_max out-of-range Exceptions. It fails on dev (UnboundLocalError) and passes with this fix.
  • Full test suite: 29 passed.

Two bugs prevented getLineProps from validating the nominal diameter
against a material's d_min / d_max bounds:

1. The range checks referenced the diameter `d` before it was assigned
   (`d = dnommm*0.001` came after the checks), so any material with
   d_min >= 0 or d_max >= 0 raised UnboundLocalError instead of the
   intended out-of-range Exception. Move the conversion above the checks.

2. loadLineProps read d_max from the key 'd_dmax' (a typo), so a
   user-specified d_max was silently dropped to the -1 'disabled'
   default and the upper-bound check never fired. Read 'd_max'.

Adds a regression test covering in-range success and both the d_min and
d_max out-of-range Exceptions.
@shousner

shousner commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

These are helpful bug fixes that were not addressed when these pieces of code were first implemented, and, as stated, do not come up during normal MoorPy operations.

@shousner shousner merged commit 004a0bc into NatLabRockies:dev Jun 4, 2026
31 checks passed
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.

2 participants