-
Notifications
You must be signed in to change notification settings - Fork 5
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
Lconstraint=-2, and pyspec higher derivatives of magnetic field #153
base: master
Are you sure you want to change the base?
Conversation
Hi Richard, Thanks for putting together this PR. A few comments:
|
Hi Elizabeth,
I think the way it is implemented, the normal field can be imposed in a fixed-boundary calculation for any Lconstraint: the change is in preset.f90, where the reading Vns/Vnc is now outside of if(Lfreebound). Am I missing something? |
Yes, I agree. I guess at the moment Vns/Vnc are read regardless of the input flags but are defaulted to zero. This seems sensible to me. I would just be careful that there are no other places where Bsups is assumed to be zero. For example, Lines 124 to 140 in 0030936
|
Good catch, thanks for pointing that out. Of course, I could just remove "if(Lconstraint .eq. -2)" and always compute Bsups, but explicitly setting it to zero seems cleaner for those cases where we want the boundary to be a flux surface. Do you (or someone else) perhaps also know how I can check the Doxygen documentation for the branch with the changes I added? I'd like to make sure the formatting worked out. Thanks, |
You can run doxygen locally on any branch you like. On GitHub, you only get the documentation from the master branch. Hope this helps :-) |
I would be ok either way. Does anyone else have an opinion on this? |
I am not going to test (yet) this branch on our machines since I see that you are probably going to make some more changes. Also, it seems to me that one should really split the "imposing the toroidal flux" option with the "imposiing a non-zero normal field on the boundary" option. The former may be called Lconstraint=-2, and the latter may simply be a flag like Lfreeboundary, e.g. Lboundarybnzero=T by default, and set to false if one wants Vns to be read and imposed at the boundary. Then, if your Lconstraint=-2 option needs to use that always, then a simple "if(Lconstraint=-2) then Lboundarybnzero=F" would suffice. What do you think? |
Thanks Jonathan! That was very helpful indeed.
I think you're right, the additional Lboundarybnzero flag is the best way to go. I will implement that now and hopefully we can get this merged then. |
… new option inputlist (Lbdybnzero)
…o only allowed with Lconstraint=-2 and Lfreebound=0; and Lconstraint=-2 only allowed with Nvol=1).
Sorry I did not see this earlier - I somehow did not receive any notification. I am satisfied with this PR, but I think @jloizu should also give his review. |
Sorry I did not review this yet, I will test compilation/execution tomorrow on our machines. Could you add (e.g. on the folder with test cases) an input file where these new options are used? |
@jloizu I have now added the test case in InputFiles/TestCases/G3V01Lm2Fi.info and .sp |
It looks like the "py_spec build" check now fails, although I only added two new test case files. Can this be caused by changes I made locally? |
Compilation works well on our machines. |
Hi @jloizu , how does the lack of force balance manifest itself? I ran the case you quoted with the master and Lconstraint=-2 executables and got very similar outputs, see file attached. |
For example when I run
while with an older version I get this
|
Hi @jloizu , |
Indeed there seems to be something already screwed up in the master branch...this is not good. I don't even understand how the previous versions passed the CI tests, since I just tried running some of them with the master branch and the code does not find equilibria...We should open an issue on this! So my guess is that your changes and branch are all fine, but there is some other more general issue... |
@jloizu @richardnies The problem @mbkumar Could you please fix the expansion of I added a few checks on the master branch in |
I think this could be because in the CI on GitLab Actions we still build SPEC using the Makefile setup, but @richardnies probably uses the CMake setup (as preferred these days)? |
Thanks Jon for debugging it. I'll work on fixing the CMake setup.
Bharat Medasani
Engineer
Princeton Plasma Physics Lab (PPPL)
…On Mon, Oct 25, 2021 at 6:09 AM Jonathan Schilling ***@***.***> wrote:
@jloizu <https://github.com/jloizu> @richardnies
<https://github.com/richardnies> The problem volume : fatal : myid= 0 ;
vflag.eq.0 .and. vvolume(lvol).lt.small ; volume cannot be zero or negative
arises on my machine if I build SPEC using the CMake setup. When built
using the traditional Makefile, it works.
The problem seems to be that in the current CMake setup the NSCREENLIST
macro is not expanded.
Thus in G3V02L1Fi.001.sp reading the screenlist fails since Wpp00aa is
not member of the namelist screenlist.
@mbkumar <https://github.com/mbkumar> Could you please fix the expansion
of NSCREENLIST in the CMake setup?
I added a few checks on the master branch in
global.f90:read_inputlists_from_file() to help debug further problems
like this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#153 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA62VEBNLH2BNXF2KDRJU6DUIUUEBANCNFSM5DX24OWA>
.
|
Actually I use the Makefile setup as well. With gfortran, if that helps. EDIT: I just saw you updated the master branch, the test runs fine now. Thanks! |
NSCREENLIST awk processing step is commented out in Makefile as well. I
introduced that change in the makefile. I need to better understand how
NSCREENLIST is used in the code.
Bharat Medasani
Engineer
Princeton Plasma Physics Lab (PPPL)
…On Tue, Oct 26, 2021 at 6:58 AM Richard Nies ***@***.***> wrote:
@jonathanschilling <https://github.com/jonathanschilling>
@jloizu <https://github.com/jloizu>
I don't even understand how the previous versions passed the CI tests,
since I just tried running some of them with the master branch and the code
does not find equilibria...We should open an issue on this!
I think this could be because in the CI on GitLab Actions we still build
SPEC using the Makefile setup, but @richardnies
<https://github.com/richardnies> probably uses the CMake setup (as
preferred these days)?
Actually I use the Makefile setup as well. With gfortran, if that helps.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#153 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA62VECU362UMVZ2NGNIMZLUI2CT7ANCNFSM5DX24OWA>
.
|
…tonUniversity/SPEC into Lconstraint_-2_and_pyspec_Zernike
I re-ran the suggested test case just now and they still seem to work. I guess we can merge this then? |
@jonathanschilling Sorry for not getting back to you earlier. One change that happened since the code changes were reviewed is that I implemented FFT's to obtain the B-field in py_spec. Should that be reviewed by someone? Apart from that, it is ready to merge on my side. |
@richardnies No worries :-) |
This branch contains three changes:
the implementation by Elizabeth Paul of Lconstraint=-2 (toroidal flux is adjusted until specified linking current is achieved), extended to work with the new Zernike version of SPEC.
possibility to specify non-zero normal component of the magnetic field in single-volume fixed-boundary calculations (through Vns/Vnc), toggled with inputlist flag Lbdybnzero (by default =.true., i.e. Bsups=0).
py_spec feature: derivatives of the magnetic field with respect to s, zeta and theta can now be obtained.