Repository navigation
TF Temperature margin improvements: calculation and de-discombobulation - #4689
Conversation
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4689 +/- ##
==========================================
+ Coverage 50.26% 50.33% +0.07%
==========================================
Files 181 181
Lines 30629 30660 +31
==========================================
+ Hits 15395 15433 +38
+ Misses 15234 15227 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d5a5cc4 to
b431ce8
Compare
CoronelBuendia
changed the title
Regression test failuresObviously, the values, bounds and residuals of constraint 36 (if present in the test input file) are now all different (A/m^2 vs. K). So large_tokamak_eval and large_tokamak_nof have 3 differences (all constraint 36 values). This means the variable name change and the T margin calculation are valid. The low_aspect_ratio_DEMO file is a little trickier to explain. Looking at just the > 5% failures for now: The ineq_con values are different, but still at residual levels, because the solver has taken a different path. This is probably because the gradients are different with the new approach (possibly less noise due to Newton solve of T margin at each iteration). Getting rid of these residual values, we see: all of which relate to the CS coil, which I'm reasonably confident I left alone here. So clearly the optimiser has settled on a different solution vector. Why this disproportionately affects the CS I am guessing because R_0 = 8.0 (lower bound) is due to ever slightly thicker TF coil (~1.3e-5 m), meaning the CS coil gets pushed inwards a bit and has to compensate to achieve the same flux. Here is the comparison (new in 57 iterations - left, old in 70 iterations - right): What the fudge pile (confirmed intermittent failure)ST eval |
CoronelBuendia
changed the title
353f95b to
c9535a7
Compare
| @ConstraintManager.register_constraint(36, "A/m²", "<=") | ||
| def constraint_equation_36(constraint_registration, data): | ||
| """Equation for TF coil s/c temperature margin lower limit (SCTF) | ||
| by means of the actual to critical current density |
There was a problem hiding this comment.
I think the description is a bit back to front now as we're limiting current density primarily maybe something like this is more accurate
Equation for TF coil superconducting current density limit at the minimum temperature margin
| minimum allowable temperature margin : TF coils (K) | ||
| j_tf_wp: | ||
| winding pack current density (A/m²) | ||
| j_tf_wp_temp_margin: |
There was a problem hiding this comment.
This isn't a temperature margin right? It's a current density, so we should change this variable name to reflect that.
There was a problem hiding this comment.
Yes, it's a current density. I named it to be in line with the other similar variables:
j_tf_wp_temp_margin : allowable TF coil winding pack current density, for specified temperature margin (A/m²), in line with j_tf_wp_critical (eqcon 33) and j_tf_wp_quench_heat_max (eqcon 35)
Happy to rename
There was a problem hiding this comment.
Morning @grmtrkngtn, thoughts on alternative name?
There was a problem hiding this comment.
Maybe j_tf_wp_temp_margin_min?
There was a problem hiding this comment.
Yes, it's a current density. I named it to be in line with the other similar variables:
j_tf_wp_temp_margin : allowable TF coil winding pack current density, for specified temperature margin (A/m²), in line withj_tf_wp_critical(eqcon 33) andj_tf_wp_quench_heat_max(eqcon 35)Happy to rename
Ok - maybe this name is fine in this context. We could just add temperature margin to the docstring?
Like
j_tf_wp_temp_margin:
superconducting strand critical current density set by temperature margin (A/m²)
There was a problem hiding this comment.
Maybe
j_tf_wp_temp_margin_min?
This makes more sense to me given the constraint docstring description
chris-ashe
left a comment
chris-ashe
left a comment
There was a problem hiding this comment.
Just a few comments and details. Looks good otherwise
| "temp_plasma_separatrix_kev": "temp_plasma_separatrix_electron_kev", | ||
| "iohcl": "i_cs_use", | ||
| "beta_total_vol_avg": None, | ||
| "tmargmin": [ |
There was a problem hiding this comment.
This list causes an error when running --update-obsolete
There was a problem hiding this comment.
That's the point, isn't it? There are multiple potential replacements, so it cannot be done automatically. Have I misunderstood what update obsolete should do, or are you suggesting that in this case we automatically populate temp_tf_superconductor_margin_min and temp_cs_superconductor_margin_min as = tmargmin and then accept any subsequent changes to those two as overrides?
There was a problem hiding this comment.
The variables should be in obsolete vars. I just think that at the moment the replacement with --update-obsolete only works when it sees a single string. It gets confused what to do with the list
| minimum allowable temperature margin : TF coils (K) | ||
| j_tf_wp: | ||
| winding pack current density (A/m²) | ||
| j_tf_wp_temp_margin: |
There was a problem hiding this comment.
Maybe
j_tf_wp_temp_margin_min?
This makes more sense to me given the constraint docstring description
| "TF coil case stress upper limit ", | ||
| "TF coil conduit stress upper limit ", | ||
| "TF coil superconductor critical current density upper limit ", | ||
| r"TF coil superconductor $j_{\mathrm{crit}}$ upper limit (ratio) ", |
There was a problem hiding this comment.
Why is "(ratio)" here in this case, The variable is still output in terms of current density
| "TF quench dump voltage upper limit ", | ||
| "TF quench hotspot current density upper limit ", | ||
| "TF coil superconductor temperature margin lower limit ", | ||
| r"TF coil superconductor $j_{\mathrm{crit}}$ upper limit (T margin) ", |
There was a problem hiding this comment.
I think these could be better named. Would still be good to have "upper" or "lower limit" at the end to make the labels standard
| """ | ||
|
|
||
| j_tf_wp_temp_margin: float = 0.0 | ||
| """j_tf_wp_temp_margin : allowable TF coil winding pack current density, for specified temperature margin (A/m²)""" |
There was a problem hiding this comment.
I am assuming this will be renamed to j_tf_wp_temp_margin_min? If so could the docstring be more specific about being the minimum allowed temp margin
| """Output TF superconductor information""" | ||
| d_sc_tf = self.data.superconducting_tfcoil | ||
|
|
||
| if self.data.tfcoil.i_str_wp == 0: |
There was a problem hiding this comment.
Am I correct in thinking that the constraint now only checks that the current density is below the minimum temp margin value and that the actual temperature margin is just calculated at output?
There was a problem hiding this comment.
Yes. The constraint checks that the WP current density <= the allowable WP current density at the minimum temperature margin.
The actual temperature margin (e.g. in the case that constraint 36 is not active) is calculated as an output. Unless, i_availability > 1, in which case the T margin is required for availability calculations.
grmtrkngtn
left a comment
grmtrkngtn
left a comment
There was a problem hiding this comment.
The modelling changes look good to me. I've tested this myself during UQ sampling and it now appears to solve robustly and correctly.
Description
Closes #4628, changing T_margin > T_margin_min to j_tf_wp < j_tf_wp_temp_margin
Closes #4687, removing duplicate/obsolete variables, and removing the inconsistent calculation of T margin and critical current density in CROCO TF coils.
Closes #3674 (for my naïve updating of obsolete variables in the input file given in the issue, this PR does not run into the error, but does fail to converge pretty quickly. Needs more testing to prove it works better.)
Performance (robustness)
Thanks to @timothy-nunn for the runs and data.
For 1000 optimisations runs on the same problem, with random starting vectors$\pm$ 20%, the PROCESS large tokamak solutions (Nb3Sn TF) go from a 53.6% success rate to a 74.0% success rate. Not all the Newton failures transform into converged solutions, but about 57% do in this case. All converged points on main also converge with this branch.
For the same setup, the PROCESS spherical tokamak solutions (REBCO Hazelton+Zhai TF) the success rate does not change (a Node crashed when running on this branch, which complicates the data, but the gist is the same). All converged points on main also converge with this branch.
Performance (convergence speed)
Thanks to @timothy-nunn for the runs and data.
Comparing only the "n_solver_iterations" for the large tokamak optimisations that were successful on main (i.e. not accounting for the additional solutions found with this branch), convergence speed is broadly unchanged:
The overall number of iterations is surprisingly low, however, probably because this problem is over-simplified and not too challenging from the optimiser perspective. I think the jury is still out on this one. I was not able to compare the run-times, but it stands to reason it will be faster with
i_availability < 2.Summary of naming / refactoring changes:
temp_marginvariable ontfcoiltmargminvariable (applied to both CS and TF coils)temp_tf_superconductor_margin_min(input) temperature margin associated with constraint 36temp_tf_superconductor_marginactual temperature marginj_tf_wp_temp_margin : allowable TF coil winding pack current density, for specified temperature margin (A/m²), in line withj_tf_wp_critical(eqcon 33) andj_tf_wp_quench_heat_max(eqcon 35)current_sharing_rebcoas this is no longer used (T margin calculated consistently based on superconductor and critical current density parameterisation)var_labelformatting inreporting/constraints.pynow accepts$ $math labelsSummary of technical changes:
calculate_superconductor_temperature_marginfrom an unbounded Newton root-finding to a bounded least squares optimisation with a check on the minimum value ~ 0.I have not looked at the CS coil, but it may benefit from similar treatment.
Checklist
I confirm that I have completed the following checks: