Fix math bug in surface computations

[unalmis]

Resolves #1174

[github-actions]

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     +0.90 +/- 7.68     | +4.57e-03 +/- 3.91e-02 |  5.13e-01 +/- 3.8e-02  |  5.09e-01 +/- 8.0e-03  |
 test_build_transform_fft_midres         |     +1.29 +/- 1.50     | +7.61e-03 +/- 8.80e-03 |  5.96e-01 +/- 6.4e-03  |  5.88e-01 +/- 6.0e-03  |
 test_build_transform_fft_highres        |     +0.32 +/- 1.83     | +3.12e-03 +/- 1.80e-02 |  9.85e-01 +/- 1.6e-02  |  9.82e-01 +/- 7.3e-03  |
 test_equilibrium_init_lowres            |     +0.32 +/- 3.55     | +1.18e-02 +/- 1.29e-01 |  3.65e+00 +/- 9.6e-02  |  3.64e+00 +/- 8.7e-02  |
 test_equilibrium_init_medres            |     +0.94 +/- 4.27     | +3.85e-02 +/- 1.74e-01 |  4.12e+00 +/- 1.2e-01  |  4.08e+00 +/- 1.2e-01  |
 test_equilibrium_init_highres           |     +0.11 +/- 0.89     | +6.13e-03 +/- 4.87e-02 |  5.50e+00 +/- 3.0e-02  |  5.50e+00 +/- 3.8e-02  |
 test_objective_compile_dshape_current   |     -0.03 +/- 1.15     | -9.83e-04 +/- 4.43e-02 |  3.86e+00 +/- 4.0e-02  |  3.86e+00 +/- 2.0e-02  |
 test_objective_compile_atf              |     +0.44 +/- 2.02     | +3.63e-02 +/- 1.67e-01 |  8.28e+00 +/- 1.4e-01  |  8.24e+00 +/- 9.0e-02  |
 test_objective_compute_dshape_current   |     +1.18 +/- 3.65     | +1.47e-05 +/- 4.55e-05 |  1.26e-03 +/- 3.3e-05  |  1.25e-03 +/- 3.1e-05  |
 test_objective_compute_atf              |     +0.52 +/- 6.10     | +2.16e-05 +/- 2.55e-04 |  4.20e-03 +/- 1.9e-04  |  4.18e-03 +/- 1.7e-04  |
 test_objective_jac_dshape_current       |     +1.00 +/- 6.02     | +3.64e-04 +/- 2.19e-03 |  3.67e-02 +/- 1.5e-03  |  3.64e-02 +/- 1.6e-03  |
 test_objective_jac_atf                  |     -2.78 +/- 3.05     | -5.28e-02 +/- 5.79e-02 |  1.85e+00 +/- 5.3e-02  |  1.90e+00 +/- 2.3e-02  |
 test_perturb_1                          |     +0.89 +/- 1.90     | +1.22e-01 +/- 2.58e-01 |  1.37e+01 +/- 1.7e-01  |  1.36e+01 +/- 1.9e-01  |
 test_perturb_2                          |     +0.95 +/- 0.89     | +1.76e-01 +/- 1.64e-01 |  1.86e+01 +/- 4.4e-02  |  1.85e+01 +/- 1.6e-01  |
 test_proximal_jac_atf                   |     -1.15 +/- 1.28     | -8.51e-02 +/- 9.40e-02 |  7.29e+00 +/- 6.9e-02  |  7.37e+00 +/- 6.3e-02  |
 test_proximal_freeb_compute             |     +0.20 +/- 1.21     | +3.60e-04 +/- 2.14e-03 |  1.77e-01 +/- 1.9e-03  |  1.77e-01 +/- 1.0e-03  |
 test_proximal_freeb_jac                 |     +0.13 +/- 1.09     | +9.18e-03 +/- 7.94e-02 |  7.30e+00 +/- 5.4e-02  |  7.29e+00 +/- 5.8e-02  |
 test_solve_fixed_iter                   |     +1.15 +/- 5.37     | +2.07e-01 +/- 9.62e-01 |  1.81e+01 +/- 7.0e-01  |  1.79e+01 +/- 6.6e-01  |

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.45%. Comparing base (db9107b) to head (90e1991).
Report is 1681 commits behind head on master.

Files with missing lines	Patch %	Lines
desc/compute/data_index.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1175      +/-   ##
==========================================
- Coverage   95.47%   95.45%   -0.03%     
==========================================
  Files          87       87              
  Lines       22310    22265      -45     
==========================================
- Hits        21301    21252      -49     
- Misses       1009     1013       +4     

Files with missing lines	Coverage Δ
desc/compute/_basis_vectors.py	100.00% <ø> (ø)
desc/compute/_core.py	100.00% <100.00%> (ø)
desc/compute/_field.py	100.00% <ø> (ø)
desc/compute/_profiles.py	99.14% <100.00%> (ø)
desc/compute/_surface.py	100.00% <ø> (ø)
desc/compute/data_index.py	95.71% <50.00%> (-1.35%)	⬇️

... and 3 files with indirect coverage changes

[YigitElma]

Ideally, we don't need repeated code for same quantity if it is calculated exactly the same as some other parametrization (as you did for some of them here). Maybe we can add a test or write a local one to see if some quantity gives different result while it should give the same. What I mean is, this bug can be spotted with the following code,

grid = LinearGrid(rho=1., M=5, N=0)
surface = eq.get_surface_at(rho=1.)
section = eq.get_surface_at(zeta=0)
keys = ["phi_z", "e_zeta"]
data = eq.compute(keys, grid=grid);
data_surf = surface.compute(keys, grid=grid)
data_sect = section.compute(keys, grid=grid)
key = "phi_z"
print(data[key])
print(data_surf[key])
print(data_sect[key])

[1. 1. 1. 1. 1. 1. 1. 1. 1. 1. 1.]
[ 8.7         8.90637041  9.45996048 10.18500929 10.85131895 11.24734087 11.24734087 10.85131895 10.18500929  9.45996048  8.90637041]
[0. 0. 0. 0. 0. 0. 0. 0. 0. 0. 0.]

Here, all should have given the same result (maybe some parameterizations have different definitions due to surface shape i.e ZernikeSurface is axisymmetric etc). It is a bit laborious to find which quantities shouldn't give the same result but I guess they are limited in number.

from desc.compute import data_index
eq_keys = data_index['desc.equilibrium.equilibrium.Equilibrium'].keys()
surf_keys = data_index['desc.geometry.surface.FourierRZToroidalSurface'].keys()
common_keys = set(eq_keys) & set(surf_keys)
len(common_keys)

For example, these 2 parameterizations have 194 common keys. Most of them should give same result when evaluated on the same linear grid.

I think something like,

grid = LinearGrid(rho=1., M=5, N=0)
surface = eq.get_surface_at(rho=1.)
keys = list(common_keys)
data = eq.compute(keys, grid=grid);
data_surf = surface.compute(keys, grid=grid)

for key in keys:
    if not np.allclose(data[key], data_surf[key]):
        print(f"{key} is not equal for surface and equilibrium")  

g_tr is not equal for surface and equilibrium
|e_zeta x e_rho| is not equal for surface and equilibrium
|e_rho x e_theta|_r is not equal for surface and equilibrium
|e_theta x e_zeta|_r is not equal for surface and equilibrium
g_rz is not equal for surface and equilibrium
e_rho is not equal for surface and equilibrium
g_zr is not equal for surface and equilibrium
|e_rho x e_theta| is not equal for surface and equilibrium
g_tt_r is not equal for surface and equilibrium
e_theta_r is not equal for surface and equilibrium
g_rr is not equal for surface and equilibrium
e_rho_z is not equal for surface and equilibrium
n_theta is not equal for surface and equilibrium
|e_theta x e_zeta|_rr is not equal for surface and equilibrium
e_zeta_r is not equal for surface and equilibrium
e_rho_t is not equal for surface and equilibrium
S is not equal for surface and equilibrium
e^theta*sqrt(g) is not equal for surface and equilibrium
g_rt is not equal for surface and equilibrium
g_tz_r is not equal for surface and equilibrium
n_zeta is not equal for surface and equilibrium

[unalmis]

Ideally, we don't need repeated code for same quantity if it is calculated exactly the same as some other parametrization (as you did for some of them here).

What do you mean? This PR removes an incorrectly computed quantity and moves computation to rely on the general method.

Regarding the rest of your comment, those bugs are due to #1127 . We can add another consistency check between objects like we do between xyz and rpz in test_compute_everything in the PR that solves #1127.

[YigitElma]

What do you mean? This PR removes an incorrectly computed quantity and moves computation to rely on the general method.

Yes, exactly. I suggested to do what you did here to solve more possible bugs.
Oh, I forgot about #1127. But in addition to removing the quantities we shouldn't be able to compute, we can also remove the duplicated code. The above code states that 173 of the 194 common computable quantities give exactly the same result.