Incremental improvements: refactors, edge cases, robustness#2
Open
Incremental improvements: refactors, edge cases, robustness#2
Conversation
Cohen-Coon formulas contain 1/theta, so calling with theta=0 silently produces inf/NaN gains. Match the existing tune_ziegler_nichols guard and raise ValueError with a units-aware message directing users to lambda tuning (which handles theta~0 correctly). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e_time The two_point FOPDT identification passes target deltas of 0.283*delta_y and 0.632*delta_y. When the step magnitude is negative (delta_y < 0) the targets are negative, but the previous crossing check assumed y[i]-y0 <= target <= y[i+1]-y0 which only holds for monotonically rising responses. Falling responses therefore raised "Response does not reach the target level" even when they clearly did. Detect the crossing by bracketing between min/max of the two samples so both directions work, and guard against zero denominators in the linear interpolation step. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nt method With fewer than 2 post-step samples the tangent branch built an empty `slopes` list and then called `max(range(0), key=...)`, which raises a bare ValueError from builtins with no context about what the caller did wrong. Check length up front and raise a targeted ValueError explaining the minimum sample count and suggesting the two_point fallback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ponse delay_steps was clamped with max(1, int(theta/dt)), which forced at least one sample of transport delay even when the FOPDT model has theta == 0. That injected a fake dt-sized lag into pure first-order simulations and biased closed-loop performance indices. Round to the nearest sample and allow delay_steps == 0 so a zero-dead-time model is simulated faithfully. Also validate dt > 0 up front to avoid a confusing ZeroDivisionError deeper in the loop. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously, passing a humidity ratio above saturation at T_db or a negative omega would send the Newton iteration outside the valid psychrometric region and eventually bubble up as a generic "did not converge within 50 iterations" RuntimeError, which is hard to diagnose for end users. Check omega >= 0 and omega <= omega_sat(T_db, P) up front and raise ValueError with all three offending quantities (with units) so the caller can see exactly which input is inconsistent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five small, atomic fixes across
control.pyandpsychrometrics.py. Each commit is independent and all 87 tests pass after every commit.Commits
fix(control): guard tune_cohen_coon against zero dead time— mirrors the existing Ziegler–Nichols guard; Cohen–Coon contains 1/theta and silently produced inf/NaN for theta=0.fix(control): support negative-step responses in _interpolate_response_time— crossing detection only worked for rising responses, soidentify_fopdt(method='two_point')failed on downward steps.fix(control): validate post-step sample count in identify_fopdt tangent method— raise a targeted error instead of a baremax(range(0))ValueError.fix(control): remove spurious one-sample dead time in closed_loop_response—max(1, int(theta/dt))injected a dt-sized lag into pure first-order models; also validate dt>0.fix(psychrometrics): validate omega feasibility in wet_bulb_temperature— supersaturated / negative omega previously surfaced as a generic non-convergence RuntimeError; now a ValueError lists omega, omega_sat, T_db, P with units.Test plan
pytest -x -qafter every commit (87 passed, ~0.05 s)