Conversation
akaptano
requested review from
AlexWiedman,
abaillod,
mishapadidar and
smhurwitz
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #483 +/- ##
==========================================
+ Coverage 89.97% 90.39% +0.42%
==========================================
Files 84 86 +2
Lines 17364 18398 +1034
==========================================
+ Hits 15623 16631 +1008
- Misses 1741 1767 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@akaptano Thanks for adding unit tests for the wireframe features! I have made a couple of changes, and I think they are now behaving as expected. |
|
@akaptano A lot of the 166 files (!!!) changed in this PR have edits that are merely just adjusting spaces or removing |
|
@landreman Will do. I assume this is because I run ./run_autopep before I commit things. Isn't this supposed to be the standard practice? @mishapadidar @mbkumar The latest version of the branch (as of this time) now has the force and torque calculations completely revamped. Unfortunately, this does not seem to fix issue #485 though it fixes #487 by removing all the BiotSavart functionality within the objective function. It also generalizes the calculation so that the list of coils can have differing number of quadrature points and so forth. Some work still to do still to fix the Taylor test issue so that the CI passes. |
|
Thanks for PR#498 with autopep, which I just merged. As a result, once master is merged into passive_coil_arrays, it should simplify this PR a lot. However when I tried to merge, there were a bunch of conflicts, and some of them I wasn't sure how to resolve. @akaptano can you merge master into passive_coil_arrays and resolve these? |
|
@landreman Just resolved those merge conflicts, thanks! We are down to 82 changed files but many of them are small edits! |
andrewgiuliani
left a comment
andrewgiuliani
left a comment
There was a problem hiding this comment.
haven't finished reading the PR, but here are some interim comments
| base_coils = coils[:ncoils] | ||
| bs = BiotSavart(coils) | ||
| bs.set_points(s.gamma().reshape((-1, 3))) | ||
| calculate_on_axis_B(bs, s) |
There was a problem hiding this comment.
it's not really the magnetic axis that you're calculating the the modB average right?
| base_curves: | ||
| The curve objects for the dipoles in the array. | ||
| eps: float | ||
| Controls how inboard a dipole can be without being removed. |
There was a problem hiding this comment.
can the docstring reflect how is a dipole classified as inboard?
can you clarify what eps actually is?
| base_curves_TF: | ||
| The curve objects for the TF (modular) coils in the array. | ||
| eps: float | ||
| controls how close a TF-dipole coil distance can be before being removed. |
There was a problem hiding this comment.
same comment as above, can the docstring reflect how interlinking coils are detected + what is eps?
| The width (radius) of a rectangular (circular) cross section dipole coil | ||
| b: float | ||
| The length (radius) of a rectangular (circular) cross section dipole coil | ||
| nturns_TF: int |
There was a problem hiding this comment.
missing regularization in the args docstring
| TF_a: minor radius of the TF coils (in R direction) | ||
| TF_b: minor radius of the TF coils (in Z direction) | ||
| fixed_geo_tfs: whether to fix the geometric degrees of freedom of the TF coils | ||
| numquadpoints: number of quadrature points representing each coil |
There was a problem hiding this comment.
order, planar_tfs missing from the docstring
| outdir: str | ||
| The output directory for the generated curves. | ||
| """ | ||
| from simsopt.geo import curves_to_vtk |
There was a problem hiding this comment.
a few of these functions import curves_to_vtk could make the code cleaner by making it a global import like numpy is
| planar_tfs: bool | ||
| Whether to use planar TF coils. | ||
| outdir: str | ||
| The output directory for the generated curves. |
There was a problem hiding this comment.
many function arguments missing from docstring
|
|
||
|
|
||
| def calculate_on_axis_B(bs, s): | ||
| def calculate_on_axis_B(bs, s, print_out=True): |
There was a problem hiding this comment.
docstring missing print_out arg
| def make_Bnormal_plots(bs, s_plot, out_dir='', bs_filename="Bnormal", B_axis=None): | ||
| """ | ||
| Plot Bnormal on plasma surface from a MagneticField object. | ||
| Do this quite a bit in the permanent magnet optimization |
There was a problem hiding this comment.
docstring missing B_axis arg
src/simsopt/_core/derivative.py
Outdated
| z[k] += y[k] | ||
| else: | ||
| z[k] = y[k].copy() | ||
| z[k] = y[k].copy() # why copy here but not in subtract? |
| @@ -22,20 +22,15 @@ class BiotSavart(sopp.BiotSavart, MagneticField): | |||
| coils: A list of :obj:`simsopt.field.coil.Coil` objects. | |||
There was a problem hiding this comment.
missing documentation for psc_array. Discuss how this affects computation.
| self._coils = coils | ||
| self.psc_array = psc_array | ||
| sopp.BiotSavart.__init__(self, coils) | ||
| MagneticField.__init__(self, depends_on=coils) |
There was a problem hiding this comment.
BiotSavart doesnt depend on psc_array?
src/simsopt/field/coil.py
Outdated
| and currents in the PSCs and the TFs. | ||
| """ | ||
|
|
||
| def __init__(self, base_psc_curves, coils_TF, eval_points, a_list, b_list, nfp=1, |
There was a problem hiding this comment.
documentation of arguments. add discussion of cross section a_list, etc
src/simsopt/field/coil.py
Outdated
| return self.get_value() | ||
|
|
||
|
|
||
| class PSCArray(): |
There was a problem hiding this comment.
generally we need consensus on having non-Optimizables
| ncoils = len(self._coils) | ||
| if any([not self.fieldcache_get_status(f'B_{i}') for i in range(ncoils)]): | ||
| assert compute_derivatives >= 0 | ||
| self.compute(compute_derivatives) |
There was a problem hiding this comment.
lets double check that we can remove this without affect to functionality. We can default to leaving it unless necessary to remove
| assert compute_derivatives >= 0 | ||
| self.compute(compute_derivatives) | ||
| self._dB_by_dcoilcurrents = [self.fieldcache_get_or_create(f'B_{i}', [npoints, 3]) for i in range(ncoils)] | ||
| return self._dB_by_dcoilcurrents |
There was a problem hiding this comment.
double check that we don't need to be setting self._dB_by_dcoilcurrents
src/simsopt/field/coil.py
Outdated
| **args | ||
| ) | ||
|
|
||
| gammas = np.array([c.gamma() for c in psc_curves]) |
There was a problem hiding this comment.
similar computation happening in recompute_currents. Should we consolidate these operations?
src/simsopt/field/coil.py
Outdated
| self.biot_savart_total.set_points(self.eval_points) | ||
| # Optimizable.__init__(self, depends_on=[self.coils, self.coils_TF]) | ||
|
|
||
| def vjp_setup(self, v_currents): |
There was a problem hiding this comment.
add dosctring describing use of this function
…ra singleton dimension.
…e sure the unit tests and examples run well again.
…. Still have some issues from newly merged code slightly changing test outputs.
…e and torque overhaul branch.
…_to_vtk, which needed fixing in all the example files.
…gain so I can better see what the current differences are with master now.
… that were different from master.
…fferences with master.
…n both dipoles and TF coils are saved jointly, with different number of quadrature points.
…ss, which seems to be required for the PSC arrays.
…nt merge from master.
… force_and_torque_overhaul branch so committing for now and will revisit. Otherwise, got the QASH scripts working better with a single wout Schuett henneberg file.
…l array examples.
…Still some fixes needed in the unit tests.
…e curveplanarellipticalcylindrical is not exported.
…stic, and fixed the dipole array example doc file.
7 participants
Hi all, this has been a long while coming, but I have my branch cleaned up that does the following:
The code is caught up and merged with several other branches, including master, Siena's coil forces branch, somewhat Jake Halpbern's dipole optimization branch, etc.