Plotting Unit Tests have missed a regression

[missing-user]

There's been a regression in the plotting functionality which wasn't detected by the unit tests.

I would strongly suggest tightening the RMS tolerances in the unit test to avoid further cases in the future.

7b6b5a3 changed the result of plot_boundaries from the baseline that is checked in

to

Since the difference is only RMS=7.5607620133305495 < tol_2d the unit tests passed
https://github.com/PlasmaControl/DESC/actions/runs/10511648755/job/29123115558
and allowed the change to merge.

Steps to reproduce

Checkout any DESC version after and run the unit tests for plotting
pytest --mpl tests/test_plotting.py::TestPlotBoundary::test_plot_boundaries
This will complete successfully although it shouldn't. In tests/test_plotting.py lower tol_2d=5 and investigate the resulting images, the red boundary only shows 3 cross sections.

Expected behavior

From the function signature and description it sounds like passing an array with 4 phi values would also plot 4 cross sections (baseline image that is checked in), but it skips the last one (second image, current behavior since 7b6b5a3). @dpanici was that the intention when creating #1204 ?

[missing-user]

I'll gladly provide a fix, but would like to confirm the expected behavior of plot_boundaries first. :)

[YigitElma]

Hi @missing-user! Thanks for pointing out this bug! Yes, the plot_boundaries function should plot 4 surfaces for the 3rd equilibrium. For others, the phi value doesn't change a thing since they are axisymmetric and correspond to the same shape. Feel free to open a PR to fix it!

@dpanici I checked to see if the NFP change to the grid caused this, but it was behaving the same, so maybe something else caused it?

[missing-user]

Thanks for the quick response!
I am pretty sure commit "simplify fix" 7b6b5a3 introduced the change because on my machine produces the error, but the parent commit b7f343d does not.

Specifically line 520 in the unit test
7b6b5a3#r147378973

Since the unit test reflects the expected usage, I suggest changing the behavior in plotting.py such that the default phi array generation doesn't include the endpoint
plotting.py:2116

    if isinstance(phi, numbers.Integral):
-        phi = np.linspace(0, 2 * np.pi / eqs[-1].NFP, phi+1)  # +1 to include pi and 2pi
+        phi = np.linspace(0, 2 * np.pi / eqs[-1].NFP, phi, endpoint=False)

So we don't have to skip the last point when plotting
plotting.py:2169

-for j in range(nz - 1):
+for j in range(nz):
     (line,) = ax.plot(
        R[:, -1, j], Z[:, -1, j], color=colors[i], linestyle=ls[i], lw=lw[i]
    )

This doesn't change the default plotting behavior (when passing an integer), and causes the expected behavior of plotting N slices when passing N phi values. It is also more consistent with the other plotting functions.

[f0uriest]

I think part of the reason this wasn't caught is that test should really be using tol_1d which is calibrated for "line plots" like this, as opposed to tol_2d which is more for filled contour plots.

[missing-user]

I think part of the reason this wasn't caught is that test should really be using tol_1d which is calibrated for "line plots" like this, as opposed to tol_2d which is more for filled contour plots.

Yeah, I agree. #1272 (comment)
Nonetheless, the change would have barely slipped below tol_1d = 7.8 > 7.5607620133305495 RMS as it is. Hence my suggestion to

1. Change this threshold to 1d
2. Lower all thresholds in general

as implemented in PR #1272