don't mod angles; they are not periodic

[unalmis]

Recall single valued functions $f$ that are periodic in $(\theta, \zeta)$ obey $f(\theta, \zeta) = f(\theta + j 2 \pi, \zeta + k 2 \pi)$. Of course $\zeta \neq \zeta + k 2 \pi$.

1. I think we should not mod any of the angles we compute $\theta, \zeta, \phi, \alpha, \vartheta$ inside our compute functions. Otherwise, we can run into issues because there are functions of interest which are not periodic. ¹

We don't mod $\phi$ right now, but if we did, attempts to map coordinates $\alpha \in \mathbf{R}, \phi \subset \mathbf{R}$ using $\alpha = \vartheta \text{mod} (2 \pi) - \iota (\phi \text{mod} (2 \pi))$ will give incorrect results because the map $\alpha \colon \vartheta, \phi \mapsto \alpha(\vartheta, \phi)$ is not periodic. Likewise since the "branch cut" (for my lack of better description) we choose to define $\alpha$ on is centered at $\phi =\vartheta = 0$, we want that

$$\lim_{\vert\phi\vert \to \infty} \vert \frac{\partial \alpha}{\partial \rho}\vert (\rho, \vartheta, \phi) = \infty \text{ when } \frac{\partial \iota}{\partial \rho} \neq 0$$

but this will not be satisfied if we mod $\phi$ in the compute function.

Currently on master we mod $\vartheta$ and $\alpha$. This doesn't cause issues for my work, but FYI it would impede computing $\phi$ through the relation $(\vartheta - \alpha) / \iota = \phi$.

2. Inside the coordinate mapping routines, we mod the coordinates by the period common to all periodic functions of those coordinates. This is fine if we only care about feeding the output to periodic functions, and should help accelerate convergence of the root finding. If we care about feeding these coordinates to non periodic functions, such as a radial derivative of $\alpha$, dependency, or chain multiple coordinate mappings together, then we could get wrong results.

I think 1. can be done without drawbacks. Not sure if 2 affects anyone. 2 affects me

Footnotes

1. For example, if we edit the compute function of $\phi = \phi mod $2\pi$, then this test will fail. Note that the output of _map_clebsch_coordinates remains correct because it never computes $\phi$, while the general map_coordinates is affected. ↩

[f0uriest]

It's probably fine to not mod things in the compute functions (or only mod them when being used in periodic ways) but we do generally need to mod both the coordinates and residuals in map_coordinates for it to converge properly

[unalmis]

Copying for future #1204 (review).

Also, another relevant instance of this. In the analytical formulas for the bounce averaged bi-normal drifts, the functions I compute are non-periodic functions of $\vartheta$, so I computed $\vartheta$ manually without the modding in the tests. If we instead relied on computing $\vartheta$ through the usual DESC compute API the test fails because of the modding.

[dpanici]

@dpanici have LinearGrid throw warning when supplied zeta exceeds 2pi/NFP

[unalmis]

@f0uriest @dpanici I have isolated my issue in #1119 to this. Although $\vert B \vert$ converged fast with a Fourier Chebyshev series, the other quantities did not. Observe

The source is the rogue $\theta$. Each time $\theta$ hits $0$ or $2\pi$ a discontinuity due to modding of the coordinate breaks the function approximation

$$f \colon \alpha, \zeta \mapsto f(\theta(\alpha, \zeta), \zeta)$$

$\Vert B \Vert$ avoided this issue because we bypass $\theta$ to compute it's spectral coefficients. Below is $\theta(\alpha, \zeta)$

I can mark #1119 for merge shortly after this is resolved.

[unalmis]

Basically, I think we want to remove this line

DESC/desc/equilibrium/coords.py

Line 444 in a4220a4

r = (alpha_k - alpha) % (2 * np.pi)