4198 raise exceptions on negative fuel ion charge density and temperature profile elements#4217
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4217 +/- ##
=======================================
Coverage 48.64% 48.64%
=======================================
Files 151 151
Lines 29703 29708 +5
=======================================
+ Hits 14449 14452 +3
- Misses 15254 15256 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR aims to surface unphysical plasma states earlier in the tokamak physics workflow by raising exceptions closer to where invalid values are produced, rather than letting them trigger harder-to-diagnose downstream failures.
Changes:
- Added an early exception when
znfuelbecomes negative during plasma composition. - Added an early exception when the Sauter bootstrap routine sees a negative temperature profile element.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
process/models/physics/physics.py |
Adds an early guard for negative znfuel in plasma composition. |
process/models/physics/bootstrap_current.py |
Adds a negative-temperature check in the Sauter bootstrap current path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if znfuel < 0.0: | ||
| raise ValueError(f"znfuel is negative: {znfuel}") |
There was a problem hiding this comment.
I disagree: we can't have regression tests for every eventuality. I don't think a unit test here adds any real benefit.
| if (tempe < 0).any(): | ||
| raise ValueError("Negative temperature in plasma profile") |
There was a problem hiding this comment.
I think this is silly, as this is would only "regress" if someone deleted this exception.
| # Check for any negative temperature in profile: always fatal eventually, | ||
| # report explicitly at source | ||
| if (tempe < 0).any(): | ||
| raise ValueError("Negative temperature in plasma profile") |
chris-ashe
left a comment
chris-ashe
left a comment
There was a problem hiding this comment.
Co-pilot is recommending adding new negative tests cases to not allow this to silently regress. Its also asking to use ProcessValueError which I think may be incorrect to do in this case
timothy-nunn
left a comment
timothy-nunn
left a comment
There was a problem hiding this comment.
Agree with Chris that co-pilot is pretty spot on. Although I think we should using a ProcessValueError here
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…harge-density-and-temperature-profile-elements
timothy-nunn
left a comment
timothy-nunn
left a comment
There was a problem hiding this comment.
Happy with changes. Will await @chris-ashe since he also reviewed before.
Both of these unphysical possibilities are always eventually fatal during model evaluation, but were previously confusing due to only causing indirect downstream errors (e.g. in bootstrap current calculation). Now they raise exceptions at source.