New Grid API #2053
New Grid API #2053
Conversation
ddudt
added
interface
desc/transform.py
Outdated
| and self.grid.NFP != self.basis.NFP | ||
| and self.basis.N != 0 | ||
| and grid.node_pattern != "custom" | ||
| and not isinstance(grid, Grid) |
There was a problem hiding this comment.
This is equivalent to the old code, since the old Grid class was hard-coded to have node_pattern="custom". In the new code, node_pattern is only an attribute of the ConcentricGrid class.
|
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 | 1.92 % | 3.843e+03 | 3.917e+03 | 73.83 | 38.77 | 35.84 |
test_proximal_jac_w7x_with_eq_update | -0.75 % | 6.614e+03 | 6.564e+03 | -49.76 | 160.17 | 158.04 |
test_proximal_freeb_jac | 0.12 % | 1.322e+04 | 1.323e+04 | 16.08 | 83.02 | 80.99 |
test_proximal_freeb_jac_blocked | -0.32 % | 7.564e+03 | 7.540e+03 | -24.30 | 72.80 | 71.30 |
test_proximal_freeb_jac_batched | 0.20 % | 7.479e+03 | 7.494e+03 | 15.05 | 72.90 | 73.29 |
test_proximal_jac_ripple | -2.18 % | 3.631e+03 | 3.552e+03 | -79.10 | 63.47 | 64.59 |
test_proximal_jac_ripple_bounce1d | -1.00 % | 3.548e+03 | 3.512e+03 | -35.59 | 74.33 | 74.43 |
test_eq_solve | -1.13 % | 2.034e+03 | 2.011e+03 | -23.03 | 92.25 | 91.33 |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.52% 93.91% -0.62%
==========================================
Files 102 107 +5
Lines 28712 29196 +484
==========================================
+ Hits 27141 27419 +278
- Misses 1571 1777 +206
🚀 New features to boost your workflow:
|
desc/grid/flux.py
Outdated
| return self.nodes.shape[0] | ||
| def period(self): | ||
| """Periodicity of coordinates.""" | ||
| if self.coordinates in ["rtz", "rvp"]: |
There was a problem hiding this comment.
I think other coordinate pairs are checked inside of Equilibrium.compute when a grid which is not an "rtz" grid is passed, in which case it calls map_coordinates with the correct arguments. So technically we support all of the combinations of
inbasis = {
"r": "rho",
"t": "theta",
"v": "theta_PEST",
"a": "alpha",
"z": "zeta",
"p": "phi",
}with outbasis being "rtz"
There was a problem hiding this comment.
OK, the x0 coordinate is always rho, and the x2 coordinate is always zeta/phi. So the only real difference is in the x1 coordinate? And then I think the only option that needs special treatment is alpha, which is the field line label instead of a polar angle. Does this look like what we want:
@property
def bounds(self):
"""Bounds of coordinates."""
if self.coordinates[1] == "a":
return ((0, 1), (0, np.inf), (0, 2 * np.pi))
else:
return ((0, 1), (0, 2 * np.pi), (0, 2 * np.pi))
@property
def period(self):
"""Periodicity of coordinates."""
if self.coordinates[1] == "a":
return (np.inf, np.inf, 2 * np.pi / self.NFP)
else:
return (np.inf, 2 * np.pi, 2 * np.pi / self.NFP)
There was a problem hiding this comment.
Yes zeta and phi should always have the same bounds and periodicity, so that shouldn't be an issue.
I think it is safe to mod vartheta when the grid is first created. This period property is only used to mod the angles when setting the grid nodes, and
There was a problem hiding this comment.
@f0uriest @dpanici is this what we decided?
@property
def bounds(self):
"""Bounds of coordinates."""
return ((0, 1), (0, 2 * np.pi), (0, 2 * np.pi))
@property
def period(self):
"""Periodicity of coordinates."""
if self.coordinates[1] == "a":
return (np.inf, np.inf, np.inf)
else:
return (np.inf, 2 * np.pi, 2 * np.pi / self.NFP)
Bounds are always
desc/grid/core.py
Outdated
| dx0 = self.bounds[0][1] - self.bounds[0][0] | ||
| dx1 = self.bounds[1][1] - self.bounds[1][0] | ||
| dx2 = self.bounds[2][1] - self.bounds[2][0] | ||
| volume = dx0 * dx1 * dx2 |
There was a problem hiding this comment.
This will be wrong for the AbstractGridCurve as is (meaning if it has (0,0) for its bounds), since there you set the bounds for x0 and x1 to (0,0) so dx0 and dx1 are zero and hence volume will be zero and so will the weights. Maybe can be trivially fixed by adding a conditional checking if each are 0 and only multiplying if they are nonzero
Techincally I guess it is "correct" as is in that the volumes of a 1D grid and 2D grid are zero, but I know you did not mean to go for that, we are going here for quadrature weights that should sum to yield the expected integral over those coordinates not necessarily "volume"
There was a problem hiding this comment.
I think, I did not check out the PR to check this myself but from just looking at it
There was a problem hiding this comment.
Thanks for catching this. See my changes and the test I added in this commit. Now I ensure that np.sum(grid.weights) is equal to the full "volume", although for 1D or 2D coordinate systems that isn't a physical 3D volume.
There was a problem hiding this comment.
Do we really want to add these new surface grid classes?
The FourierRZToroidalSurface already has an attribute rho and its default compute grid is:
LinearGrid(rho=np.array(self.rho), M=2 * self.M + 5, N=2 * self.N + 5, NFP=self.NFP)
So we would probably also need to remove that rho attribute, and then it gets complicated to tell when surfaces are flux surfaces or generic geometric surfaces.
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 ABCAbstractRTZGridis 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 appropriateGrid->CustomGridFlux, etc. (do not deprecate)