Skip to content

Fix FourierCoeffs copy/move span rebinding#7

Open
jungdaesuh wants to merge 5 commits intomainfrom
fix-issue-164-long-term
Open

Fix FourierCoeffs copy/move span rebinding#7
jungdaesuh wants to merge 5 commits intomainfrom
fix-issue-164-long-term

Conversation

@jungdaesuh
Copy link
Owner

@jungdaesuh jungdaesuh commented Jan 9, 2026 •
edited
Loading

Summary
Addresses Issue proximafusion#164 (span aliasing) by rebinding views after copy/move/assignment in FourierCoeffs-derived classes.

This PR also includes a small CI compatibility fix in src/vmecpp/simsopt_compat.py for change_resolution() return semantics.

Why this approach vs. deleting copy/move
Deleting copy/move changes semantics. This keeps copy/move/assignment behavior while ensuring derived spans rebind to owned storage.

Current status (as of February 6, 2026)

  • PR changes 9 files, including src/vmecpp/simsopt_compat.py.
  • Current head commit is 07741285cdf86392e03287b0c5c13bb171d87c32.
  • CI is green (23/23 checks passing).
  • C++ opt/asan/ubsan jobs run bazel build //..., bazel build //vmecpp_large_cpp_tests/..., and bazel test //...; logs report Executed 30 out of 38 tests: 38 tests pass.

CI-fix scope split

  • C++: clang-tidy bugprone-use-after-move fixes in Fourier C++ move paths.
  • Python: simsopt compatibility fix in src/vmecpp/simsopt_compat.py (change_resolution return handling).

Regression test coverage note

  • This PR does not add regression test files directly.
  • The CopyAndMoveRebindSpans regression test exists in:
    • jungdaesuh/vmecpp_large_cpp_tests#1
    • proximafusion/vmecpp_large_cpp_tests#7
  • Current CI checks out proximafusion/vmecpp_large_cpp_tests main (commit 2da1138... in the current run), so that PR-branch regression test is not yet included in upstream CI.

Related PRs

Files

  • src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.h
  • src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.cc
  • src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.h
  • src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc
  • src/vmecpp/cpp/vmecpp/vmec/fourier_velocity/fourier_velocity.h
  • src/vmecpp/cpp/vmecpp/vmec/fourier_velocity/fourier_velocity.cc
  • src/vmecpp/cpp/vmecpp/vmec/fourier_forces/fourier_forces.h
  • src/vmecpp/cpp/vmecpp/vmec/fourier_forces/fourier_forces.cc
  • src/vmecpp/simsopt_compat.py

Design choice

  • I considered two approaches:
    • A) Minimal fix (this PR): explicit assignment handling + rebinding derived spans after copy/move.
    • B) Fundamental redesign: rule-of-zero refactor (remove stored span members and non-assignable config members, expose spans via accessors).
  • I chose A here to keep scope small and avoid C++ API breakage.
  • If preferred, I can implement B in a follow-up PR.

github-actions[bot]
github-actions bot reviewed Jan 9, 2026
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.cc Outdated

int num_fc_RZ = (jMaxIncludingBoundary - nsMin) * s_.mpol * (s_.ntor + 1);
int num_fc_L = (jMaxIncludingBoundary - nsMin) * s_.mpol * (s_.ntor + 1);
int num_fc_RZ = (jMaxIncludingBoundary - nsMin) * s_->mpol * (s_->ntor + 1);
Copy link

@github-actions github-actions bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for variable 'num_fc_RZ' [readability-identifier-naming]

Suggested change
int num_fc_RZ = (jMaxIncludingBoundary - nsMin) * s_->mpol * (s_->ntor + 1);
int num_fc_rz = (jMaxIncludingBoundary - nsMin) * s_->mpol * (s_->ntor + 1);

src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.cc:27:

-   rcc.resize(num_fc_RZ, 0.0);
-   zsc.resize(num_fc_RZ, 0.0);
+   rcc.resize(num_fc_rz, 0.0);
+   zsc.resize(num_fc_rz, 0.0);

src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.cc:31:

-     rss.resize(num_fc_RZ, 0.0);
-     zcs.resize(num_fc_RZ, 0.0);
+     rss.resize(num_fc_rz, 0.0);
+     zcs.resize(num_fc_rz, 0.0);

src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.cc:36:

-     rsc.resize(num_fc_RZ, 0.0);
-     zcc.resize(num_fc_RZ, 0.0);
+     rsc.resize(num_fc_rz, 0.0);
+     zcc.resize(num_fc_rz, 0.0);

src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.cc:40:

-       rcs.resize(num_fc_RZ, 0.0);
-       zss.resize(num_fc_RZ, 0.0);
+       rcs.resize(num_fc_rz, 0.0);
+       zss.resize(num_fc_rz, 0.0);

src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.cc Outdated
int num_fc_RZ = (jMaxIncludingBoundary - nsMin) * s_.mpol * (s_.ntor + 1);
int num_fc_L = (jMaxIncludingBoundary - nsMin) * s_.mpol * (s_.ntor + 1);
int num_fc_RZ = (jMaxIncludingBoundary - nsMin) * s_->mpol * (s_->ntor + 1);
int num_fc_L = (jMaxIncludingBoundary - nsMin) * s_->mpol * (s_->ntor + 1);
Copy link

@github-actions github-actions bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for variable 'num_fc_L' [readability-identifier-naming]

Suggested change
int num_fc_L = (jMaxIncludingBoundary - nsMin) * s_->mpol * (s_->ntor + 1);
int num_fc_l = (jMaxIncludingBoundary - nsMin) * s_->mpol * (s_->ntor + 1);

src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.cc:29:

-   lsc.resize(num_fc_L, 0.0);
+   lsc.resize(num_fc_l, 0.0);

src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.cc:33:

-     lcs.resize(num_fc_L, 0.0);
+     lcs.resize(num_fc_l, 0.0);

src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.cc:38:

-     lcc.resize(num_fc_L, 0.0);
+     lcc.resize(num_fc_l, 0.0);

src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.cc:42:

-       lss.resize(num_fc_L, 0.0);
+       lss.resize(num_fc_l, 0.0);

src/vmecpp/cpp/vmecpp/vmec/fourier_coefficients/fourier_coefficients.h Outdated
const int ns;
int nsMin_;
int nsMax_;
int ns;
Copy link

@github-actions github-actions bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for protected member 'ns' [readability-identifier-naming]

Suggested change
int ns;
int ns_;

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc Outdated
// weighting factor for radial interpolation between 0 at axis and 1
// at boundary
double interpolationWeight = pow(p.sqrtSF[jF - r_.nsMinF1], m);
double interpolationWeight = pow(p.sqrtSF[jF - r_->nsMinF1], m);
Copy link

@github-actions github-actions bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for variable 'interpolationWeight' [readability-identifier-naming]

Suggested change
double interpolationWeight = pow(p.sqrtSF[jF - r_->nsMinF1], m);
double interpolation_weight = pow(p.sqrtSF[jF - r_->nsMinF1], m);

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:116:

-           rmncc[idx_fc] = basis_norm * interpolationWeight * b.rbcc[idx_bdy];
-           zmnsc[idx_fc] = basis_norm * interpolationWeight * b.zbsc[idx_bdy];
+           rmncc[idx_fc] = basis_norm * interpolation_weight * b.rbcc[idx_bdy];
+           zmnsc[idx_fc] = basis_norm * interpolation_weight * b.zbsc[idx_bdy];

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:119:

-             rmnss[idx_fc] = basis_norm * interpolationWeight * b.rbss[idx_bdy];
-             zmncs[idx_fc] = basis_norm * interpolationWeight * b.zbcs[idx_bdy];
+             rmnss[idx_fc] = basis_norm * interpolation_weight * b.rbss[idx_bdy];
+             zmncs[idx_fc] = basis_norm * interpolation_weight * b.zbcs[idx_bdy];

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:123:

-             rmnsc[idx_fc] = basis_norm * interpolationWeight * b.rbsc[idx_bdy];
-             zmncc[idx_fc] = basis_norm * interpolationWeight * b.zbcc[idx_bdy];
+             rmnsc[idx_fc] = basis_norm * interpolation_weight * b.rbsc[idx_bdy];
+             zmncc[idx_fc] = basis_norm * interpolation_weight * b.zbcc[idx_bdy];

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:127:

-                   basis_norm * interpolationWeight * b.rbcs[idx_bdy];
+                   basis_norm * interpolation_weight * b.rbcs[idx_bdy];

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:129:

-                   basis_norm * interpolationWeight * b.zbss[idx_bdy];
+                   basis_norm * interpolation_weight * b.zbss[idx_bdy];

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc Outdated
std::vector<double> rmnss_at_jF(s_.mpol * (s_.ntor + 1));
fb.cos_to_cc_ss(rmnc_row_vector, rmncc_at_jF, rmnss_at_jF, s_.ntor,
s_.mpol);
std::vector<double> rmncc_at_jF(s_->mpol * (s_->ntor + 1));
Copy link

@github-actions github-actions bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for variable 'rmncc_at_jF' [readability-identifier-naming]

Suggested change
std::vector<double> rmncc_at_jF(s_->mpol * (s_->ntor + 1));
std::vector<double> rmncc_at_j_f(s_->mpol * (s_->ntor + 1));

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:156:

-     fb.cos_to_cc_ss(rmnc_row_vector, rmncc_at_jF, rmnss_at_jF, s_->ntor,
+     fb.cos_to_cc_ss(rmnc_row_vector, rmncc_at_j_f, rmnss_at_jF, s_->ntor,

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:171:

-         rmncc[idx_jmn] = rmncc_at_jF[idx_mn];
+         rmncc[idx_jmn] = rmncc_at_j_f[idx_mn];

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc Outdated
const int idx_jmn = ((jF - nsMin_) * s_.mpol + m) * (s_.ntor + 1) + n;
std::vector<double> zmnsc_at_jF(s_->mpol * (s_->ntor + 1));
std::vector<double> zmncs_at_jF(s_->mpol * (s_->ntor + 1));
fb.sin_to_sc_cs(zmns_row_vector, zmnsc_at_jF, zmncs_at_jF, s_->ntor,
Copy link

@github-actions github-actions bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for variable 'zmncs_at_jF' [readability-identifier-naming]

Suggested change
fb.sin_to_sc_cs(zmns_row_vector, zmnsc_at_jF, zmncs_at_jF, s_->ntor,
std::vector<double> zmncs_at_j_f(s_->mpol * (s_->ntor + 1));
fb.sin_to_sc_cs(zmns_row_vector, zmnsc_at_jF, zmncs_at_j_f, s_->ntor,

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:175:

-           zmncs[idx_jmn] = zmncs_at_jF[idx_mn];
+           zmncs[idx_jmn] = zmncs_at_j_f[idx_mn];

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc Outdated
std::vector<double> lmncs_at_jF(s_.mpol * (s_.ntor + 1));
fb.sin_to_sc_cs(lmns_row_vector, lmnsc_at_jF, lmncs_at_jF, s_.ntor,
s_.mpol);
std::vector<double> lmnsc_at_jF(s_->mpol * (s_->ntor + 1));
Copy link

@github-actions github-actions bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for variable 'lmnsc_at_jF' [readability-identifier-naming]

Suggested change
std::vector<double> lmnsc_at_jF(s_->mpol * (s_->ntor + 1));
std::vector<double> lmnsc_at_j_f(s_->mpol * (s_->ntor + 1));

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:191:

-     fb.sin_to_sc_cs(lmns_row_vector, lmnsc_at_jF, lmncs_at_jF, s_->ntor,
+     fb.sin_to_sc_cs(lmns_row_vector, lmnsc_at_j_f, lmncs_at_jF, s_->ntor,

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:204:

-         lmnsc[idx_jmn] = lmnsc_at_jF[idx_mn] / lambda_unscaling;
+         lmnsc[idx_jmn] = lmnsc_at_j_f[idx_mn] / lambda_unscaling;

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc Outdated
s_.mpol);
std::vector<double> lmnsc_at_jF(s_->mpol * (s_->ntor + 1));
std::vector<double> lmncs_at_jF(s_->mpol * (s_->ntor + 1));
fb.sin_to_sc_cs(lmns_row_vector, lmnsc_at_jF, lmncs_at_jF, s_->ntor,
Copy link

@github-actions github-actions bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for variable 'lmncs_at_jF' [readability-identifier-naming]

Suggested change
fb.sin_to_sc_cs(lmns_row_vector, lmnsc_at_jF, lmncs_at_jF, s_->ntor,
std::vector<double> lmncs_at_j_f(s_->mpol * (s_->ntor + 1));
fb.sin_to_sc_cs(lmns_row_vector, lmnsc_at_jF, lmncs_at_j_f, s_->ntor,

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:206:

-           lmncs[idx_jmn] = lmncs_at_jF[idx_mn] / lambda_unscaling;
+           lmncs[idx_jmn] = lmncs_at_j_f[idx_mn] / lambda_unscaling;

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc Outdated
int firstSurface1 = (firstSurface * s_.mpol + m1) * (s_.ntor + 1) + n;
int axis0 = (axis * s_->mpol + m0) * (s_->ntor + 1) + n;
int axis1 = (axis * s_->mpol + m1) * (s_->ntor + 1) + n;
int firstSurface0 = (firstSurface * s_->mpol + m0) * (s_->ntor + 1) + n;
Copy link

@github-actions github-actions bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for variable 'firstSurface0' [readability-identifier-naming]

Suggested change
int firstSurface0 = (firstSurface * s_->mpol + m0) * (s_->ntor + 1) + n;
int first_surface0 = (firstSurface * s_->mpol + m0) * (s_->ntor + 1) + n;

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:301:

-       lmncs[axis0] = lmncs[firstSurface0];
+       lmncs[axis0] = lmncs[first_surface0];

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:309:

-       lmncc[axis0] = lmncc[firstSurface0];
+       lmncc[axis0] = lmncc[first_surface0];

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc Outdated
int axis0 = (axis * s_->mpol + m0) * (s_->ntor + 1) + n;
int axis1 = (axis * s_->mpol + m1) * (s_->ntor + 1) + n;
int firstSurface0 = (firstSurface * s_->mpol + m0) * (s_->ntor + 1) + n;
int firstSurface1 = (firstSurface * s_->mpol + m1) * (s_->ntor + 1) + n;
Copy link

@github-actions github-actions bot Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for variable 'firstSurface1' [readability-identifier-naming]

Suggested change
int firstSurface1 = (firstSurface * s_->mpol + m1) * (s_->ntor + 1) + n;
int first_surface1 = (firstSurface * s_->mpol + m1) * (s_->ntor + 1) + n;

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:292:

-     rmncc[axis1] = rmncc[firstSurface1];
-     zmnsc[axis1] = zmnsc[firstSurface1];
-     lmnsc[axis1] = lmnsc[firstSurface1];
+     rmncc[axis1] = rmncc[first_surface1];
+     zmnsc[axis1] = zmnsc[first_surface1];
+     lmnsc[axis1] = lmnsc[first_surface1];

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:296:

-       rmnss[axis1] = rmnss[firstSurface1];
-       zmncs[axis1] = zmncs[firstSurface1];
-       lmncs[axis1] = lmncs[firstSurface1];
+       rmnss[axis1] = rmnss[first_surface1];
+       zmncs[axis1] = zmncs[first_surface1];
+       lmncs[axis1] = lmncs[first_surface1];

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:304:

-       rmnsc[axis1] = rmnsc[firstSurface1];
-       zmncc[axis1] = zmncc[firstSurface1];
-       lmncc[axis1] = lmncc[firstSurface1];
+       rmnsc[axis1] = rmnsc[first_surface1];
+       zmncc[axis1] = zmncc[first_surface1];
+       lmncc[axis1] = lmncc[first_surface1];

src/vmecpp/cpp/vmecpp/vmec/fourier_geometry/fourier_geometry.cc:311:

-         rmncs[axis1] = rmncs[firstSurface1];
-         zmnss[axis1] = zmnss[firstSurface1];
-         lmnss[axis1] = lmnss[firstSurface1];
+         rmncs[axis1] = rmncs[first_surface1];
+         zmnss[axis1] = zmnss[first_surface1];
+         lmnss[axis1] = lmnss[first_surface1];

@jungdaesuh jungdaesuh force-pushed the fix-issue-164-long-term branch 6 times, most recently from cf74da6 to b25868a Compare January 10, 2026 04:37
@jungdaesuh jungdaesuh force-pushed the fix-issue-164-long-term branch from b25868a to 56ec23c Compare February 5, 2026 05:24
@jungdaesuh jungdaesuh force-pushed the fix-issue-164-long-term branch from 56ec23c to 1765fcc Compare February 5, 2026 05:31
@jungdaesuh
Copy link
Owner Author

jungdaesuh commented Feb 6, 2026 •
edited
Loading

I evaluated two options for proximafusion#164:

  • A) Minimal fix (current PR): explicit assignment handling + span rebinding in derived Fourier types.
  • B) Fundamental redesign (rule-of-zero): remove stored span members and non-assignable config members, expose spans via accessors, and rely on compiler-generated special members.

I chose A for this PR because it fixes the stale-span bug with minimal scope and no C++ API break. If preferred, I can implement B in a follow-up PR. Any preference?

jungdaesuh and others added 3 commits February 11, 2026 01:48
@jungdaesuh @jurasic-pf
Fix simsopt compatibility with change_resolution (proximafusion#402)
f604ace 
* Fix simsopt compat for resolution change

* Fix simsopt compat and VMEC2000 comparison

* Limit VMEC2000 comparison to VmecInput fields

* Use class variable instead of dunder method

---------

Co-authored-by: Philipp Jurašić <166746189+jurasic-pf@users.noreply.github.com>
@jurasic-pf
Merge branch 'main' into fix-issue-164-long-term
782bb21
@jurasic-pf
Update simsopt_compat.py
7c5cb68
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jungdaesuh @jurasic-pf