Change iteration variables to prevent negative winding pack thickness
Closed this issue · 15 comments
In GitLab by @mkovari on May 10, 2017, 15:48
Please note: we should all be using thkcas and thkwp as iteration variables! @Hlux @kellis @jmorris-uk @stuartmuldrew @mkumar
At the moment we somtimes get negative winding pack thickness.
This is because the total TF coil thickness tfcth and the TF nose thickness thkcas are both iteration variables. The winding pack thickness results from subtracting them:
thkwp = tfcth - casthi - thkcas - 2.0D0*tinstf - 2.0d0*tfinsgap
There is nothing to stop this going negative.
The simplest approach is to swap so that thkcas and thkwp are iteration variables. Then they will add rather than subtract when calculating the total thickness tfcth. (We would probably add a line in initial.f90 to ensure that tfcth is defined correctly at the start.)
(It may be possible instead to create a new constraint to ensure that tfcth is always bigger than thkcas, but if the constraint residual is not calculated until after the code crashes then there is not much point.)
I have made it backwards compatible, with a new iteration variable 140. The following code appears in machine_build.f90:
! Calculate tfcth if thkwp is an iteration variable (140)
if (any(ixc(1:nvar) == 140) ) then
tfcth = thkwp + casthi + thkcas + 2.0D0*tinstf + 2.0d0*tfinsgap
endif
and this in sctfcoil.f90:
if (any(ixc(1:nvar) == 140) ) then
tfcth = thkwp + casthi + thkcas + 2.0D0*tinstf + 2.0d0*tfinsgap
else
thkwp = tfcth - casthi - thkcas - 2.0D0*tinstf - 2.0d0*tfinsgap
endif
In GitLab by @rkemp on May 10, 2017, 16:12
Would almost certainly break backwards compatibility with non-SC coils, but I'm not sure that's something we're concerned about.
Could experiment first by adding thkwp as an iteration variable and replacing the line you quote with the summed version at the appropriate place in sctf and check it runs correctly (with tfcth not an iteration variable). I think you're right about needing something in initial.f90; can't remember the calculation order but this is needed before the radial build is determined, which may be before the magnet calculations, so it might be more appropriate to include it there instead.
In GitLab by @rkemp on May 10, 2017, 16:33
Should also check whether the stellarator model uses it, and fix it in the same way if so.
In GitLab by @mkovari on May 11, 2017, 16:09
After a long battle I followed your advice and included the code in machine_build and now it works!
In GitLab by @mkovari on May 11, 2017, 16:15
I have not looked at the stellarator or the resistive coil code.
In GitLab by @mkovari on May 11, 2017, 16:16
In due course we can delete the annoying error reporting code from sctfcoil as it should now never be needed.
Note that there is no warning if you select tfcth and thkwp as iteration variables, but the results will be unpredictable.
We should remove the option of having tfcth as an iteration variable, after the test suite input files have been fixed.
Does anyone volunteer to fix the test suite and remove the backwards compatibility?
@Hlux @kellis @stuartmuldrew @jmorris-uk @mkumar
In GitLab by @mkovari on May 11, 2017, 16:20
mentioned in commit c9350e89fa25869b79580f9e2e0292c0f2dd70d8
In GitLab by @mkovari on Dec 5, 2018, 08:24
The code seems to have disappeared from sctfcoil.f90. @jmorris-uk Could you investigate?
Commit 44e5af28 authored 7 months ago by James Morris
sctfcoil.f90
In GitLab by @Hlux on Dec 5, 2018, 08:47
While James is travelling and is away, you can try git blame to find out in which commit this has happened.
In GitLab by @mkovari on May 1, 2020, 09:41
This issue is still open and it seems that the code was reverted without explanation (see comment above)!!!
We should remove the option of having tfcth as an iteration variable.
Note, however, Richard's comment: "Would almost certainly break backwards compatibility with non-SC coils..."
Does anyone volunteer to sort this out?
@stuartmuldrew @ajpearcey @jonmaddock , but especially @jmorris-uk.
@schislet Sorry about this - a bit of a mess!!
In GitLab by @jmorris-uk on May 1, 2020, 11:37
mentioned in commit b1891c3c9425a4c3ec00bda244c603c1e326a5a6
In GitLab by @jmorris-uk on May 1, 2020, 14:40
I've reverted the code to the logic that was agreed in this issue. All subsequent changes should be made via a new issue.
This doesn't break the resistive coil winding pack as it now is calculated in a separate subroutine.
In GitLab by @jmorris-uk on May 1, 2020, 14:40
closed
In GitLab by @mkovari on Dec 9, 2022, 15:27
Hi James - can you clarify what your last comment means? Can you describe how the code works and whether it prevents negative values?
In GitLab by @mkovari on Dec 9, 2022, 15:27
reopened