-
Notifications
You must be signed in to change notification settings - Fork 17
Remove f values #3947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Remove f values #3947
Conversation
c6ca56b to
484d46e
Compare
484d46e to
56efc15
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3947 +/- ##
==========================================
+ Coverage 46.03% 46.22% +0.19%
==========================================
Files 123 123
Lines 28970 28774 -196
==========================================
- Hits 13335 13301 -34
+ Misses 15635 15473 -162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1bd708f to
6302f99
Compare
665329e to
cfacb82
Compare
| ixc = 46 | ||
| fp_hcd_injected_max = 1.0 | ||
| boundl(46) = 0.6 | ||
| boundu(46) = 1.5 | ||
| * DESCRIPTION: f-value for injected power variation range | ||
| * JUSTIFICATION: Setup to allow the injected power to vary | ||
|
|
||
| p_hcd_injected_max = 150.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When fp_hcd_injected_max = 1.5:
| *--------------------------Numerics Variables---------------------------* | ||
| *‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾* | ||
|
|
||
| epsvmc = 1.0E-11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was much lower than other regression tests
How to update an input file to not use f-values
In 99% of cases, this will allow your file to run without f-values and without treating inequality constraints as equality. However, if the input file specified weird bounds on the f-value iteration variables, you will need to modify the constraint bound (max/min) to account for this change. |
mkovari
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments
|
|
||
|
|
||
| !!! Info "Constraints" | ||
| A full list of constraints is given on the variable description page in the row labelled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update this in due course.
| OBS_VARS_HELP.update( | ||
| dict.fromkeys( | ||
| fvalues_list, | ||
| "F-values are no longer supported, please use inequality constraints", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the inequality constraints are documented we could add a link to the docs here.
process/pfcoil.py
Outdated
| numerics.boundu[37] | ||
| * pfcoil_variables.j_cs_critical_flat_top_end | ||
| # numerics.boundu[fjohc] * (which should have been 1) | ||
| pfcoil_variables.j_cs_critical_flat_top_end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this can ever be false:
abs(pfcoil_variables.j_cs_flat_top_end) > 0.99e0 * abs(pfcoil_variables.j_cs_critical_flat_top_end)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this warning (cslimit).
Keep fjohc and fjohc0 as variables (not iteration) and include them in the constraint.
Use them instead of the bounds in this line of code.
| "CS not using max current density: further optimisation may be possible" | ||
| ) | ||
|
|
||
| # Check whether CS coil currents are feasible from engineering POV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally these warnings should be recreated but it's no big deal.
process/stellarator.py
Outdated
|
|
||
| # The operation current density weighted with the global iop/icrit fraction | ||
| lhs[:] = constraint_variables.fiooic * jcrit_vector | ||
| lhs[:] = jcrit_vector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't really correct anymore, I suspect, and the same may be true for the code below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the recourse here then? Should fiooic be added back? Or is there a way to calculate it from other variables in PROCESS (maybe the mentioned iop/icrit)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I really don't know anything about the stellarator code. I don't know what lhs and rhs are.
|
|
||
| This constraint can be activated by stating `icc = 15` in the input file. | ||
|
|
||
| The scaling value `fl_h_threshold (ixc=103)` can be varied to set the required margin around the threshold. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fl_h_threshold should be kept and renamed. This new way of using it should be documented here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the description and docs to document how I think fl_h_threshold is working now. We should probably go through this at some point (with some example input files) to ensure its working as before/expected.
In particular, we should carefully check how it was used to enforce L-mode/H-mode before.
cfacb82 to
c9b3d71
Compare
|
Thank you both for the reviews. I have resolved everything that I have actioned. The following comments are still outstanding:
|
|
@chris-ashe has pointed me to Looking at an MFile, they have very similar values: So I can replace the warning and its use in stellarator with This will not work for the Stellarator (#3947 (comment)) because |
|
There are two separate constraint equations for enforcing the L-H threshold: icc = 15 and 73. |
@ajpearcey @chris-ashe @jonmaddock do we have any thoughts on this? I'm not sure we can use I think this requires the margin (f-value) to be iterating. |
process/stellarator.py
Outdated
|
|
||
| # The operation current density weighted with the global iop/icrit fraction | ||
| lhs[:] = constraint_variables.fiooic * jcrit_vector | ||
| lhs[:] = jcrit_vector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I really don't know anything about the stellarator code. I don't know what lhs and rhs are.
process/pfcoil.py
Outdated
| numerics.boundu[37] | ||
| * pfcoil_variables.j_cs_critical_flat_top_end | ||
| # numerics.boundu[fjohc] * (which should have been 1) | ||
| pfcoil_variables.j_cs_critical_flat_top_end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this warning (cslimit).
Keep fjohc and fjohc0 as variables (not iteration) and include them in the constraint.
Use them instead of the bounds in this line of code.
| "j_cs_pulse_start / j_cs_critical_pulse_start shouldn't be above 0.7 " | ||
| "for engineering reliability" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These warnings are fine. Remember the constraints on current density may not be applied (if T margin is used instead).
| "f_c_tf_turn_operating_critical shouldn't be above 0.7 for engineering reliability" | ||
| ) | ||
|
|
||
| po.ovarre( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This warning is fine.
| args : output structure : residual error; constraint value; | ||
| fiooic: f-value for TF coil operating current / critical | ||
| j_tf_wp_critical: critical current density for winding pack (A/m2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need fiooic:
The current density margin can be set using fiooic.
d7f5b38 to
f4d507b
Compare
| This constraint can be activated by stating `icc = 5` in the input file. | ||
|
|
||
| The value of `i_density_limit` can be set to apply the relevant limit . The scaling value `fdene` can be varied also. | ||
| The value of `i_density_limit` can be set to apply the relevant limit. The variable `fdene` can be set to scale the constraint bound: `nd_plasma_electrons_vol_avg` / `nd_plasma_electrons_max` <= `fdene`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i_density_limit isnt always limits that apply to the volume averaged density.
| $$ | ||
|
|
||
| **Therefore it is recommended to always use `icc = 15` if trying to simulate a plasma scenario specifically in L or H-mode** | ||
| Again, `fl_h_threshold` can be used to add a margin to the constraint. For example, `fl_h_threshold = 1.25` ensures that `p_plasma_separatrix_mw` is at least 25% less than the threshold power. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it not be 25% more?
| ****************** | ||
| nsweep = 17 * b_tf_inboard_max, maximum peak toroidal field | ||
| isweep = 6 | ||
| sweep = 10.5, 10.4, 10.3, 10.2, 10.1, 10.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know how close the solution is initially to this inboard leg limit? Would it be better to have a scan example that directly changes a variable instead of a limit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for this PR I will leave it but we should probably create an issue to make a more meaningful example
process/constraints.py
Outdated
| ) | ||
|
|
||
|
|
||
| @ConstraintManager.register_constraint(22, "MW", ">=") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of numerical sanity should this not be further down?
|
Chris also recommends having two separate margins for constraint 15 and 22 |
|
Hi All, I have added in the two separate variables and updated the docstrings/docs accordingly. Please check my work carefully to ensure this new way of constraining on the LH threshold makes sense. I have had to make some changes in |
| \mathtt{fl\_h\_threshold} \times \underbrace{\mathtt{p\_l\_h\_threshold\_mw}}_{\text{Power from scaling}} >= \mathtt{p\_plasma\_separatrix\_mw} | ||
| $$ | ||
|
|
||
| **Therefore it is recommended to always use `icc = 15` if trying to simulate a plasma scenario specifically in L or H-mode** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really icc = 15 is mandatory (unless a different mode is selected), since the plasma must be in some mode.
| data_structure.current_drive_variables.n_beam_decay_lengths_core_required * cc, | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this here all of a sudden?
(Equation to fix number of NBI decay lengths to plasma centre)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the equations around so that they were in order
| - data_structure.physics_variables.p_l_h_threshold_mw | ||
| / data_structure.constraint_variables.l_mode_threshold_margin, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code really is a cumbersome way to write a simple equation.
It would be easier to read if each of the return quantities had a name.
The code is fine but I think you may need to rename the margin. l_mode_threshold_margin is a fraction, not a margin. If l_mode_threshold_margin = 0.8 then the margin is 20%.

Removes F-values from PROCESS
st_regression.IN.DATused some weird f-value bounds so I have updated the constraints (see comment below).