Skip to content

Fix RampInterpolator double-counting and calc_knownRp pressure conversion#19

Merged
Ickaser merged 3 commits into
LyoHUB:mainfrom
SECQUOIA:pr/fix-ramp-and-pressure
Mar 30, 2026
Merged

Fix RampInterpolator double-counting and calc_knownRp pressure conversion#19
Ickaser merged 3 commits into
LyoHUB:mainfrom
SECQUOIA:pr/fix-ramp-and-pressure

Conversation

@bernalde
Copy link
Copy Markdown
Contributor

Summary

  • Fix RampInterpolator with count_ramp_against_dt=True and no init key double-counting dt_setpt[0], and clamp negative holdtime to 0
  • Fix calc_knownRp.dry early-return path reporting pressure in Torr instead of mTorr
  • Fix several test issues: self-comparison assertion, hasattr() on dict, floating-point tolerance, Python <3.11 compatibility

Test plan

  • New regression tests for RampInterpolator fix (test_ramp_interpolator_noinit_multisetpt_no_double_count, test_ramp_interpolator_holdtime_clamped_to_zero)
  • New regression test for pressure conversion (TestCalcKnownRpEarlyReturn)
  • All 140 existing tests pass

Closes #17

🤖 Generated with Claude Code

…conversion

Fix two bugs:
1. RampInterpolator with count_ramp_against_dt=True and no 'init' key
   double-counted dt_setpt[0], and did not clamp negative holdtime.
2. calc_knownRp.dry early-return path reported pressure in Torr instead
   of mTorr, inconsistent with normal output path.

Also fixes several test issues:
- Fix self-comparison assertion in test_freezing.py
- Fix hasattr() on dict (should use 'in') in optimizer tests
- Add tolerance for floating-point differences in optimizer reference tests
- Fix contextlib.chdir compatibility for Python < 3.11

Closes #17

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@Ickaser Ickaser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked it over. A few suggestions, some comments should be more terse and I don't like introducing tolerances for the ramp interpolation which should not encounter much (if any) numerical noise.

Comment thread tests/conftest.py Outdated
Comment thread tests/test_calc_knownRp.py Outdated
Comment thread tests/test_calc_knownRp.py Outdated
Comment thread tests/test_opt_Pch.py Outdated
Comment thread tests/test_opt_Pch.py
Comment thread tests/test_opt_Pch.py
Comment thread tests/test_opt_Pch_Tsh.py Outdated
Comment thread lyopronto/functions.py Outdated
@bernalde
Copy link
Copy Markdown
Contributor Author

Addressed the review in 3a75c92. That commit removes the unused fixture, shortens the RampInterpolator comment, makes the early-return mTorr regression check exact, makes the opt_Pch max-bound check exact, and fixes the missing Pchamber["max"] enforcement in opt_Pch_Tsh while tightening the corresponding test. I did not remove the opt_Pch reference-time tolerance in tests/test_opt_Pch.py, because the exact comparison against reference_opt_Pch.csv already differs on this branch by 4.406960061764664e-06 hr (4.238515035519084 vs 4.238510628559022) without a behavioral change in the optimizer. The PR-relevant test slice passes after these changes: 114 passed, 1 skipped from pytest tests/test_functions.py tests/test_calc_knownRp.py tests/test_freezing.py tests/test_main.py tests/test_opt_Pch.py tests/test_opt_Pch_Tsh.py tests/test_opt_Tsh.py.

@Ickaser Ickaser merged commit 0377079 into LyoHUB:main Mar 30, 2026
3 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.

RampInterpolator: potential double-counting of first stage duration in no-init path

2 participants