Update lablcc labels#4266
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the human-readable constraint labels (lablcc) and a few constraint registration/docstring strings to use clearer terminology and more standard fusion-physics notation (subscripts, angle brackets, etc.) in PROCESS’s constraint reporting/output.
Changes:
- Refreshed
process/data_structure/numerics.py:lablccstrings to more descriptive/symbolic labels. - Updated constraint registration strings in
process/core/solver/constraints.py(e.g., volt-second unit label) and tweaked some docstring text/notation. - Simplified constraint 51’s implementation to use
vs_plasma_ramp_requireddirectly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
process/data_structure/numerics.py |
Updates the lablcc constraint label list to newer, more descriptive/symbolic names. |
process/core/solver/constraints.py |
Adjusts selected constraint registration strings and docstrings (including volt-second unit labeling) and refactors constraint 51 to use vs_plasma_ramp_required. |
Comments suppressed due to low confidence (1)
process/data_structure/numerics.py:599
- Constraints 69–71 are not registered in
process/core/solver/constraints.py(the registry jumps from 68 to 72), butlablccstill provides labels for them (with# REMOVED). Consider using a consistent placeholder like "NOT USED"/blank here to reflect that these IDs are intentionally unavailable, and to reduce confusion when reading constraint listings.
"p_sep < psep_kallenbach divertor ", # REMOVED
"Separatrix temp consistency ", # REMOVED
"Separatrix density consistency ", # REMOVED
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "Vessel helium concentration limit", # REMOVED | ||
| "Pₛₑₚ / R₀ upper limit ", | ||
| "TF coil leg rad width lower limit", # REMOVED | ||
| "TF coil leg rad width lower limit", # REMOVED |
There was a problem hiding this comment.
I think this is a good point here, as it is misleading to have proper initial values for constraints that don't exist, so replacing them with a 'NOT USED' placeholder would be good
| @@ -1194,15 +1194,11 @@ def constraint_equation_50(constraint_registration, data): | |||
| def constraint_equation_51(constraint_registration, data): | |||
| """Equation to enforce startup flux = available startup flux | |||
|
|
|||
| vs_plasma_res_ramp: resistive losses in startup V-s (Wb) | |||
| vs_plasma_ind_ramp: internal and external plasma inductance V-s (Wb)) | |||
| vs_plasma_ramp_required: Required flux swing for startup (Wb) | |||
| vs_cs_pf_total_ramp: total flux swing for startup (Wb) | |||
| """ | |||
| return eq( | |||
| abs( | |||
| data_structure.physics_variables.vs_plasma_res_ramp | |||
| + data_structure.physics_variables.vs_plasma_ind_ramp | |||
| ), | |||
| abs(data_structure.physics_variables.vs_plasma_ramp_required), | |||
| data.pf_coil.vs_cs_pf_total_ramp, | |||
There was a problem hiding this comment.
I agree with this comment
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4266 +/- ##
=======================================
Coverage 50.13% 50.13%
=======================================
Files 151 151
Lines 29343 29349 +6
=======================================
+ Hits 14710 14714 +4
- Misses 14633 14635 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
clmould
left a comment
There was a problem hiding this comment.
My review for now, but will get a modeller to check as well as they know the constraints and labels better than I do!
| @@ -1194,15 +1194,11 @@ def constraint_equation_50(constraint_registration, data): | |||
| def constraint_equation_51(constraint_registration, data): | |||
| """Equation to enforce startup flux = available startup flux | |||
|
|
|||
| vs_plasma_res_ramp: resistive losses in startup V-s (Wb) | |||
| vs_plasma_ind_ramp: internal and external plasma inductance V-s (Wb)) | |||
| vs_plasma_ramp_required: Required flux swing for startup (Wb) | |||
| vs_cs_pf_total_ramp: total flux swing for startup (Wb) | |||
| """ | |||
| return eq( | |||
| abs( | |||
| data_structure.physics_variables.vs_plasma_res_ramp | |||
| + data_structure.physics_variables.vs_plasma_ind_ramp | |||
| ), | |||
| abs(data_structure.physics_variables.vs_plasma_ramp_required), | |||
| data.pf_coil.vs_cs_pf_total_ramp, | |||
There was a problem hiding this comment.
I agree with this comment
| data_structure.physics_variables.vs_plasma_res_ramp | ||
| + data_structure.physics_variables.vs_plasma_ind_ramp | ||
| ), | ||
| abs(data_structure.physics_variables.vs_plasma_ramp_required), |
There was a problem hiding this comment.
Why has this been changed from vs_plasma_res_ramp + vs_plasma_ind_ramp to vs_plasma_ramp_required ?
There was a problem hiding this comment.
As vs_plasma_ramp_required is the sum of the two. Thus prevents useless extra calculations and room for errors
| "Vessel helium concentration limit", # REMOVED | ||
| "Pₛₑₚ / R₀ upper limit ", | ||
| "TF coil leg rad width lower limit", # REMOVED | ||
| "TF coil leg rad width lower limit", # REMOVED |
There was a problem hiding this comment.
I think this is a good point here, as it is misleading to have proper initial values for constraints that don't exist, so replacing them with a 'NOT USED' placeholder would be good
| "TF turn dimension upper limit ", | ||
| "Cryogenic plant power upper limit ", | ||
| "TF WP vertical strain upper limit ", | ||
| "CS current to copper area upper limit ", |
There was a problem hiding this comment.
Does the word 'fraction' or 'ratio' need to go in here to replace the '/' from before? Or keep the / like in line 603 ?
| "CS Tresca yield criterion upper limit ", | ||
| "Pₛₑₚ > Pₗₕ + Pₐᵤₓ consistency ", | ||
| "TF quench temperature < temp_croco_quench_max", | ||
| "TF current/copper area < Max ", |
There was a problem hiding this comment.
Other lines are now using 'upper limit' instead of 'max' - would this make sense here too for consistency?
There was a problem hiding this comment.
I have left those for the breakout PR after the CroCo refactor goes in
Co-authored-by: clmould <86794332+clmould@users.noreply.github.com>
…s.py and constraints.py
j-a-foster
left a comment
There was a problem hiding this comment.
Everything looks good to me.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
process/data_structure/numerics.py:544
- This label embeds an inequality (
Pₛₑₚ > Pₗₕ) even though the solver output already includes the constraint symbol (this one is implemented as>=). To avoid contradictory or duplicated information in tables/logs, consider removing the>from the label (or use the same operator as the registered constraint).
"Burn time lower limit ",
"NBI decay lengths consistency ",
"Pₛₑₚ > Pₗₕ consistency ",
"Net electric power lower limit ",
process/data_structure/numerics.py:602
- Same issue as constraint 15’s label: embedding
Pₛₑₚ > Pₗₕ + Pₐᵤₓin the name duplicates the separate constraint symbol column and uses a strict>while the constraint itself is implemented as>=. Consider removing the inequality from the label (or matching the operator exactly).
"NOT USED", # REMOVED
"CS Tresca yield criterion upper limit ",
"Pₛₑₚ > Pₗₕ + Pₐᵤₓ consistency ",
"TF quench temperature < temp_croco_quench_max",
| "Electron power balance ", | ||
| "Density upper limit ", | ||
| "(Epsilon x beta-pol) upper limit ", | ||
| "Electron density upper limit (nₑ<) ", |
| def constraint_equation_51(constraint_registration, data): | ||
| """Equation to enforce startup flux = available startup flux | ||
|
|
||
| vs_plasma_res_ramp: resistive losses in startup V-s (Wb) | ||
| vs_plasma_ind_ramp: internal and external plasma inductance V-s (Wb)) | ||
| vs_plasma_ramp_required: Required flux swing for startup (Wb) | ||
| vs_cs_pf_total_ramp: total flux swing for startup (Wb) | ||
| """ | ||
| return eq( | ||
| abs( | ||
| data_structure.physics_variables.vs_plasma_res_ramp | ||
| + data_structure.physics_variables.vs_plasma_ind_ramp | ||
| ), | ||
| abs(data_structure.physics_variables.vs_plasma_ramp_required), | ||
| data.pf_coil.vs_cs_pf_total_ramp, |
| "Neutron wall load upper limit ", | ||
| "Fusion power upper limit ", | ||
| "Toroidal field 1/R consistency ", | ||
| "NOT USED", # REMOVED |
There was a problem hiding this comment.
I think just having NOT USED is enough, so the REMOVED can be removed now
Description
Checklist
I confirm that I have completed the following checks: