Conversation
ddudt
added
interface
|
Add checks in objectives and inside geometric/magneticfield/coil compute methods to ensure grids passed in are correct (i.e. 1D for a filament coil, 3D for finite build coil, 1D or 2D rtz for surface) |
|
Rename AbstractRTZGrid to AbstractGridFlux instead of RTZ |
Memory benchmark result| Test Name | %Δ | Master (MB) | PR (MB) | Δ (MB) | Time PR (s) | Time Master (s) |
| -------------------------------------- | ------------ | ------------------ | ------------------ | ------------ | ------------------ | ------------------ |
test_objective_jac_w7x | -0.79 % | 4.044e+03 | 4.012e+03 | -31.99 | 33.45 | 30.74 |
test_proximal_jac_w7x_with_eq_update | 1.61 % | 6.522e+03 | 6.627e+03 | 105.12 | 153.69 | 155.95 |
test_proximal_freeb_jac | 0.32 % | 1.333e+04 | 1.337e+04 | 42.28 | 80.03 | 79.93 |
test_proximal_freeb_jac_blocked | 0.06 % | 7.665e+03 | 7.669e+03 | 4.38 | 72.61 | 71.60 |
test_proximal_freeb_jac_batched | -0.06 % | 7.638e+03 | 7.634e+03 | -4.33 | 72.41 | 68.89 |
test_proximal_jac_ripple | -2.01 % | 3.582e+03 | 3.510e+03 | -71.97 | 56.58 | 54.55 |
test_proximal_jac_ripple_bounce1d | 1.05 % | 3.741e+03 | 3.781e+03 | 39.37 | 70.14 | 67.21 |
test_eq_solve | 3.19 % | 1.965e+03 | 2.028e+03 | 62.73 | 58.38 | 58.00 |For the memory plots, go to the summary of |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2053 +/- ##
==========================================
- Coverage 94.32% 94.09% -0.24%
==========================================
Files 101 106 +5
Lines 28870 29403 +533
==========================================
+ Hits 27231 27666 +435
- Misses 1639 1737 +98
🚀 New features to boost your workflow:
|
| eq = self.things[0] | ||
| if self._grid is None: | ||
| grid = LinearGrid( | ||
| grid = LinearGridFlux( |
There was a problem hiding this comment.
I don't totally understand why LinearGrid(M=M,N=N) and LinearGrid(N=N) have been separated into different classes, but LinearGrid(L=L) is a specific instance of LinearGridFlux and not a separate class.
There was a problem hiding this comment.
In this case we use a LinearGridFlux because the Pressure profile is evaluated in flux coordinates, even though it typically only cares about the
We might want to make a new 1D grid class for "profiles", similar to AbstractGridCurve but in the range
There was a problem hiding this comment.
Looks great; I left a couple small comments but I have no real changes to recommend. The only major question (which doesn't need to be solved in this PR) I have is about FFT/DCT; we should generalize how grids that can FFT/DCT in a given coordinate/subset of coordinates are denoted, so that we can have more general transform methods.
There was a problem hiding this comment.
Looks great; I left a couple small comments but I have no real changes to recommend. The only major question (which doesn't need to be solved in this PR) I have is about FFT/DCT; we should generalize how grids that can FFT/DCT in a given coordinate/subset of coordinates are denoted, so that we can have more general transform methods.
YigitElma
left a comment
YigitElma
left a comment
There was a problem hiding this comment.
It looks good to me.
We are again about to hit 2 month mark since the last release. I was thinking of maybe having a quick release after merging PRs #2264, #2266, #2267, #2268 and maybe #2247 and #2262. I know this sounds a lot but all of them are easy and they improve the performance of a lot of things. I suggest this because we were planning to use this PR in master before making the release. Let me know what you think.
Alternatively, we can have a release couple weeks after merging this one.
| M, theta, theta_period, endpoint, sym=self.sym | ||
| ) | ||
| # zeta | ||
| z, dz, self._fft_x2 = _create_linear_nodes( |
There was a problem hiding this comment.
On master it throws an error if users create a LinearGrid with zeta values stretching beyond 2pi/NFP, whereas here it just mods those values. Should this be mentioned in the docstring?
There was a problem hiding this comment.
Do we actually want this to be an error? phi=4pi is physically equivalent to phi=0 or 2pi. I need to check if this causes issues in the Transform class.
There was a problem hiding this comment.
Is there a reason why bases are still hardcoded to be in flux coordinates?
| @property | ||
| def fft_toroidal(self): | ||
| """bool: whether this basis is compatible with fft in the toroidal direction.""" | ||
| if not hasattr(self, "_fft_toroidal"): |
There was a problem hiding this comment.
See comment above; shouldn't fft_poloidal and fft_toroidal also be updated to be fft_x1 and fft_x2, or should that be a separate PR? If so, the generalization of bases and transforms should occur before #2253 since the cylindrical grid classes introduced there are mostly useful if they have corresponding bases and transforms.
There was a problem hiding this comment.
I tried to not change much of the Basis classes in this PR to keep the scope limited (this is already a very large PR). But yes, we should generalize the bases to not assume flux coordinates and I don't think it is a difficult update. Not every basis class directly correspond to a single grid class, but the basis doesn't need much info about the grids. Can you make this change in #2253?
8 participants
Resolves #1840
Restructures and generalizes the Grid classes. The new ABC$\rho,\theta,\zeta$ ) coordinates and is the parent class of all existing grid classes in flux coordinates. Most of the changes involved moving existing code into one of these new ABCs.
AbstractGridthat all other grids inherit from does not assume a particular coordinate system. The new ABCAbstractGridFluxis specific to (The goal is to allow for new grid classes in the future that represent other coordinate systems, without changing the existing public API.
To-Do:
Basisdimensions (no references to flux coords)isinstance(grid, AbstractGridCurve)checks wherever appropriateCurveobjectsFourierRZToroidalSurfaceobjectsGrid->CustomGridFlux, etc. (do not deprecate)docs/dev_guide/notebooks/grid.ipynbLinearGrid._create_nodes?