Skip to content

Commit

Permalink
add issues to TODOs or remove
Browse files Browse the repository at this point in the history
  • Loading branch information
dpanici committed Nov 18, 2024
1 parent 95612fa commit f8c64e6
Show file tree
Hide file tree
Showing 34 changed files with 62 additions and 68 deletions.
4 changes: 2 additions & 2 deletions desc/basis.py
Original file line number Diff line number Diff line change
Expand Up @@ -1098,13 +1098,13 @@ def evaluate(
if not len(modes):
return np.array([]).reshape((len(nodes), 0))

# TODO: avoid duplicate calculations when mixing derivatives
# TODO(#1243): avoid duplicate calculations when mixing derivatives
r, t, z = nodes.T
l, m, n = modes.T
lm = modes[:, :2]

if unique:
# TODO: can avoid this here by using grid.unique_idx etc
# TODO(#1243): can avoid this here by using grid.unique_idx etc
# and adding unique_modes attributes to basis
_, ridx, routidx = np.unique(
r, return_index=True, return_inverse=True, axis=0
Expand Down
1 change: 0 additions & 1 deletion desc/batching.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ def f_(carry, x):
return res_append


# TODO in_axes a la vmap?
def _scanmap(fun, scan_fun, argnums=0):
"""A helper function to wrap f with a scan_fun."""

Expand Down
4 changes: 2 additions & 2 deletions desc/coils.py
Original file line number Diff line number Diff line change
Expand Up @@ -1901,8 +1901,8 @@ def save_in_makegrid_format(self, coilsFilename, NFP=None, grid=None):
if None, will default to the coil compute functions's
default grid
"""
# TODO: name each group based off of CoilSet name?
# TODO: have CoilGroup be automatically assigned based off of
# TODO(#1376): name each group based off of CoilSet name?
# TODO(#1376): have CoilGroup be automatically assigned based off of
# CoilSet if current coilset is a collection of coilsets?

NFP = 1 if NFP is None else NFP
Expand Down
2 changes: 1 addition & 1 deletion desc/compute/_omnigenity.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def fitfun(x):
return data


# TODO: do math to change definition of nu so that we can just use B_zeta_mn here
# TODO (#568): do math to change definition of nu so that we can just use B_zeta_mn here
@register_compute_fun(
name="B_phi_mn",
label="B_{\\phi, m, n}",
Expand Down
6 changes: 3 additions & 3 deletions desc/compute/_profiles.py
Original file line number Diff line number Diff line change
Expand Up @@ -1378,7 +1378,7 @@ def _iota_num_rrr(params, transforms, profiles, data, **kwargs):
- beta * data["sqrt(g)_rrr"],
data["sqrt(g)"],
),
# Todo: axis limit of beta_rrr
# TODO(#587): axis limit of beta_rrr
# Computed with four applications of l’Hôpital’s rule.
# Requires sqrt(g)_rrrr and fourth derivatives of basis vectors.
jnp.nan,
Expand Down Expand Up @@ -1656,7 +1656,7 @@ def _iota_den_rrr(params, transforms, profiles, data, **kwargs):
- gamma * data["sqrt(g)_rrr"],
data["sqrt(g)"],
),
# Todo: axis limit
# TODO(#587): axis limit
# Computed with four applications of l’Hôpital’s rule.
# Requires sqrt(g)_rrrr and fourth derivatives of basis vectors.
jnp.nan,
Expand Down Expand Up @@ -1713,7 +1713,7 @@ def _q(params, transforms, profiles, data, **kwargs):
return data


# TODO: add K(rho,theta,zeta)*grad(rho) term
# TODO (#1381): add K(rho,theta,zeta)*grad(rho) term
@register_compute_fun(
name="I",
label="I",
Expand Down
2 changes: 1 addition & 1 deletion desc/compute/_stability.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def _D_current(params, transforms, profiles, data, **kwargs):
/ data["|grad(psi)|"] ** 3
* dot(Xi, data["B"])
),
# Todo: implement equivalent of equation 4.3 in desc coordinates
# TODO(#671): implement equivalent of equation 4.3 in desc coordinates
jnp.nan,
)
)
Expand Down
1 change: 0 additions & 1 deletion desc/continuation.py
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,6 @@ def solve_continuation( # noqa: C901
if len(deltas) > 0:
if verbose > 0:
print("Perturbing equilibrium")
# TODO: pass Jx if available
eqp = eqfam[ii - 1].copy()
objective_i = get_equilibrium_objective(
eq=eqp, mode=objective, jac_chunk_size=jac_chunk_size
Expand Down
4 changes: 2 additions & 2 deletions desc/equilibrium/coords.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ def map_coordinates( # noqa: C901

profiles = get_profiles(inbasis + basis_derivs, eq)

# TODO: make this work for permutations of in/out basis
# TODO (#1382): make this work for permutations of in/out basis
if outbasis == ("rho", "theta", "zeta"):
if inbasis == ("rho", "alpha", "zeta"):
if "iota" in kwargs:
Expand Down Expand Up @@ -766,7 +766,7 @@ def get_rtz_grid(
return desc_grid


# TODO: deprecated, remove eventually
# TODO(#1383): deprecated, remove eventually
def compute_theta_coords(
eq, flux_coords, L_lmn=None, tol=1e-6, maxiter=20, full_output=False, **kwargs
):
Expand Down
4 changes: 2 additions & 2 deletions desc/equilibrium/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ def parse_axis(axis, NFP=1, sym=True, surface=None):
name="axis",
)
elif axis is None: # use the center of surface
# TODO: make this method of surface, surface.get_axis()?
# TODO (#1384): make this method of surface, surface.get_axis()?
if isinstance(surface, FourierRZToroidalSurface):
axis = FourierRZCurve(
R_n=surface.R_lmn[np.where(surface.R_basis.modes[:, 1] == 0)],
Expand All @@ -160,7 +160,7 @@ def parse_axis(axis, NFP=1, sym=True, surface=None):
NFP=NFP,
)
elif isinstance(surface, ZernikeRZToroidalSection):
# TODO: include m=0 l!=0 modes
# TODO (#782): include m=0 l!=0 modes
axis = FourierRZCurve(
R_n=surface.R_lmn[
np.where(
Expand Down
2 changes: 1 addition & 1 deletion desc/geometry/surface.py
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ def from_input_file(cls, path, **kwargs):
)
return surf

# TODO: add k value for number of rotations per field period
# TODO (#1385): add k value for number of rotations per field period
@classmethod
def from_qp_model(
cls,
Expand Down
1 change: 0 additions & 1 deletion desc/input_reader.py
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,6 @@ def parse_inputs(self, fname=None): # noqa: C901
if match:
inputs["bdry_mode"] = words[0].lower()
flag = True
# TODO: set bdry_mode automatically based on bdry coeffs

# coefficient indices
match = re.search(r"l\s*:\s*" + num_form, command, re.IGNORECASE)
Expand Down
5 changes: 3 additions & 2 deletions desc/integrals/bounce_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def _check_spline_shape(knots, g, dg_dz, pitch_inv=None):
to that field line.
"""
errorif(knots.ndim != 1, msg=f"knots should be 1d; got shape {knots.shape}.")
errorif(knots.ndim != 1, msg=f"knots should be 1d, got shape {knots.shape}.")
errorif(
g.shape[-2] != (knots.size - 1),
msg=(
Expand Down Expand Up @@ -390,7 +390,8 @@ def loop(z): # over num well axis
)

result = jnp.moveaxis(
# TODO: Use batch_size arg of imap after increasing JAX version requirement.
# TODO (#1386): Use batch_size arg of imap after
# increasing JAX version requirement.
imap(loop, (jnp.moveaxis(z1, -1, 0), jnp.moveaxis(z2, -1, 0)))[1],
source=0,
destination=-1,
Expand Down
2 changes: 1 addition & 1 deletion desc/integrals/interp_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def polyval_vec(*, x, c):
)


# TODO: Eventually do a PR to move this stuff into interpax.
# TODO (#1388): Eventually do a PR to move this stuff into interpax.


def _subtract_last(c, k):
Expand Down
6 changes: 2 additions & 4 deletions desc/integrals/surface_integral.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
from desc.grid import ConcentricGrid, LinearGrid
from desc.utils import errorif, warnif

# TODO: Make the surface integral stuff objects with a callable method instead of
# returning functions. Would make simpler, allow users to actually see the
# TODO (#1389): Make the surface integral stuff objects with a callable method instead
# of returning functions. Would make simpler, allow users to actually see the
# docstrings of the methods, and less bookkeeping to default to more
# efficient methods on tensor product grids.

Expand Down Expand Up @@ -227,8 +227,6 @@ def surface_integrals_map(grid, surface_label="rho", expand_out=True, tol=1e-14)
)
spacing = jnp.prod(spacing, axis=1)

# Todo: Define mask as a sparse matrix once sparse matrices are no longer
# experimental in jax.
if has_idx:
# The ith row of masks is True only at the indices which correspond to the
# ith surface. The integral over the ith surface is the dot product of the
Expand Down
2 changes: 1 addition & 1 deletion desc/magnetic_fields/_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -2401,7 +2401,7 @@ def __init__(
assert len(x_lmn) == self.x_basis.num_modes
self._x_lmn = x_lmn

# TODO: should we not allow some types of helicity?
# TODO (#1390): should we not allow some types of helicity?
helicity_sign = sign(helicity[0]) * sign(helicity[1])
warnif(
self.helicity != (0, self.NFP * helicity_sign)
Expand Down
10 changes: 5 additions & 5 deletions desc/magnetic_fields/_dommaschk.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def fit_magnetic_field( # noqa: C901
B = field(coords)
else: # it must be the field evaluated at the passed-in coords
B = field
# TODO: add basis argument for if passed-in field or callable
# TODO (#928): add basis argument for if passed-in field or callable
# evaluates rpz or xyz basis magnetic field vector,
# and what basis coords is

Expand All @@ -132,7 +132,7 @@ def fit_magnetic_field( # noqa: C901
# b is made, now do A
#####################
num_modes = 1 + (max_l) * (max_m + 1) * 4
# TODO: if symmetric, technically only need half the modes
# TODO (#928): if symmetric, technically only need half the modes
# however, the field and functions are setup to accept equal
# length arrays for a,b,c,d, so we will just zero out the
# modes that don't fit symmetry, but in future
Expand All @@ -141,7 +141,7 @@ def fit_magnetic_field( # noqa: C901
# and the modes array can then be [m,l,x] where x is 0,1,2,3
# and we dont need to keep track of a,b,c,d separately

# TODO: technically we can drop some modes
# TODO (#928): technically we can drop some modes
# since if max_l=0, there are only ever nonzero terms for a and b
# and if max_m=0, there are only ever nonzero terms for a and c
# but since we are only fitting in a least squares sense,
Expand Down Expand Up @@ -247,7 +247,7 @@ def get_B_dom(coords, X, ms, ls):

# now solve Ac=b for the coefficients c

# TODO: use min singular value to give sense of cond number?
# TODO (#928): use min singular value to give sense of cond number?
c, res, _, _ = jnp.linalg.lstsq(A, rhs)

if verbose > 0:
Expand All @@ -258,7 +258,7 @@ def get_B_dom(coords, X, ms, ls):
B0 = c[0]

# we zero out the terms that should be zero due to symmetry here
# TODO: should also just not return any zeroed-out modes, but
# TODO (#928): should also just not return any zeroed-out modes, but
# the way the modes are cataloged here with the ls and ms arrays,
# it is not straightforward to do that
a_arr = c[1 : n + 1] * abcd_zero_due_to_sym_inds[0]
Expand Down
9 changes: 3 additions & 6 deletions desc/objectives/linear_objectives.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from .objective_funs import _Objective


# TODO: get rid of this class and inherit from FixParameters instead?
# TODO (#1391): get rid of this class and inherit from FixParameters instead?
class _FixedObjective(_Objective):
_fixed = True
_linear = True
Expand All @@ -45,8 +45,8 @@ def update_target(self, thing):
def _parse_target_from_user(
self, target_from_user, default_target, default_bounds, idx
):
# TODO: add logic here to deal with `target_from_user` as a pytree?
# TODO: does this actually need idx?
# TODO (#1391): add logic here to deal with `target_from_user` as a pytree?
# TODO (#1391: does this actually need idx?
if target_from_user is None:
target = default_target
bounds = default_bounds
Expand Down Expand Up @@ -2513,7 +2513,6 @@ def __init__(
normalize_target=normalize_target,
name=name,
)
# TODO: add normalization?


class FixCurveRotation(FixParameters):
Expand Down Expand Up @@ -2879,7 +2878,6 @@ def __init__(
normalize_target=normalize_target,
name=name,
)
# TODO: add normalization?


class FixOmniMap(FixParameters):
Expand Down Expand Up @@ -3094,7 +3092,6 @@ def __init__(
normalize_target=normalize_target,
name=name,
)
# TODO: add normalization?


class FixNearAxisR(_FixedObjective):
Expand Down
5 changes: 2 additions & 3 deletions desc/objectives/nae_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -295,18 +295,17 @@ def _make_RZ_cons_order_rho( # noqa: C901
the O(rho) behavior of the equilibrium R coefficients to match the NAE.
Zconstraints : tuple of Objective
tuple of constraints of type FixSumModesZ, which enforce
the O(rho) behavior of the equilibrium Z coefficents to match the NAE.
the O(rho) behavior of the equilibrium Z coefficients to match the NAE.
Lconstraints : tuple of Objective
tuple of constraints of type FixSumModesLambda, which enforce
the O(rho) behavior of the equilibrium lambda coefficents to match the NAE.
the O(rho) behavior of the equilibrium lambda coefficients to match the NAE.
Tuple is empty if fix_lambda=False.
"""
if qsc is not None:
# r is the ratio r_NAE / rho_DESC
r = np.sqrt(2 * abs(desc_eq.Psi / qsc.Bbar) / 2 / np.pi)
else:
# TODO: is this true?
r = 1 # using DESC equilibrium's behavior, no conversion is needed

Rconstraints = ()
Expand Down
2 changes: 1 addition & 1 deletion desc/objectives/objective_funs.py
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ def unpack_state(self, x, per_objective=True):

def x(self, *things):
"""Return the full state vector from the Optimizable objects things."""
# TODO: also check resolution etc?
# TODO (#1392): also check resolution of the things etc?
things = things or self.things
errorif(
len(things) != len(self.things),
Expand Down
4 changes: 2 additions & 2 deletions desc/optimize/_constraint_wrappers.py
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ def unpack_state(self, x, per_objective=True):

def x(self, *things):
"""Return the full state vector from the Optimizable objects things."""
# TODO: also check resolution etc?
# TODO (#1392): also check resolution etc?
things = things or self.things
assert [type(t1) is type(t2) for t1, t2 in zip(things, self.things)]
xs = []
Expand Down Expand Up @@ -872,7 +872,7 @@ def grad(self, x, constants=None):
gradient vector.
"""
# TODO: figure out projected vjp to make this better
# TODO (#1393): figure out projected vjp to make this better
f = jnp.atleast_1d(self.compute_scaled_error(x, constants))
J = self.jac_scaled_error(x, constants)
return f.T @ J
Expand Down
2 changes: 1 addition & 1 deletion desc/optimize/_desc_wrappers.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ def _optimize_desc_aug_lagrangian(
ub,
lambda x, *c: constraint.jac_scaled(x, c[1]),
)
# TODO: can't pass constants dict into vjp for now
# TODO (#1394): can't pass constants dict into vjp for now
constraint_wrapped.vjp = lambda v, x, *args: constraint.vjp_scaled(v, x)
else:
constraint_wrapped = None
Expand Down
2 changes: 1 addition & 1 deletion desc/optimize/least_squares.py
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ def lsqtr( # noqa: C901
)
alltr.append(trust_radius)
alpha *= tr_old / trust_radius
# TODO: does this need to move to the outer loop?
# TODO (#1395): does this need to move to the outer loop?
success, message = check_termination(
actual_reduction,
cost,
Expand Down
2 changes: 1 addition & 1 deletion desc/plotting.py
Original file line number Diff line number Diff line change
Expand Up @@ -3340,7 +3340,7 @@ def plot_basis(basis, return_data=False, **kwargs):
"""
title_fontsize = kwargs.pop("title_fontsize", None)

# TODO: add all other Basis classes
# TODO(#1377): add all other Basis classes
if basis.__class__.__name__ == "PowerSeries":
grid = LinearGrid(rho=100, endpoint=True)
r = grid.nodes[:, 0]
Expand Down
Loading

0 comments on commit f8c64e6

Please sign in to comment.