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.

https://github.com/PlasmaControl/DESC/commit/7b6b5a35bf40e41ff37600920c886a47e95b79bd 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 https://github.com/PlasmaControl/DESC/commit/7b6b5a35bf40e41ff37600920c886a47e95b79bd). @dpanici was that the intention when creating https://github.com/PlasmaControl/DESC/pull/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" https://github.com/PlasmaControl/DESC/commit/7b6b5a35bf40e41ff37600920c886a47e95b79bd introduced the change because on my machine produces the error, but the parent commit https://github.com/PlasmaControl/DESC/commit/b7f343dd98726f67243a72f610d70dec281646c0 does not.

Specifically line 520 in the unit test https://github.com/PlasmaControl/DESC/commit/7b6b5a35bf40e41ff37600920c886a47e95b79bd#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. https://github.com/PlasmaControl/DESC/pull/1272#discussion_r1780979553 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 https://github.com/PlasmaControl/DESC/pull/1272