Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zeta_B computation is sometimes wrong #1163

Closed
unalmis opened this issue Aug 5, 2024 · 3 comments · Fixed by #1166
Closed

zeta_B computation is sometimes wrong #1163

unalmis opened this issue Aug 5, 2024 · 3 comments · Fixed by #1166
Assignees
Labels
bug Something isn't working

Comments

@unalmis
Copy link
Collaborator

unalmis commented Aug 5, 2024

Sometimes when I rerun the test that compares everything is computed the same as master up to floating point errors, I get significant errors on zeta_B.

Not equal to tolerance rtol=1e-10, atol=1e-10
Parameterization: desc.magnetic_fields._core.OmnigenousField. Name: zeta_B.
Mismatched elements: 270 / 297 (90.9%)
Max absolute difference: 11.42397329
Max relative difference: 34.0466585
 x: array([ 0.      ,  0.550532,  1.076765,  1.557329,  1.976363,  2.325429,
        2.604526,  2.822094,  2.993993,  0.      ,  0.550532,  1.076765,
        1.557329,  1.976363,  2.325429,  2.604526,  2.822094,  2.993993,...
 y: array([0.      , 0.550532, 1.076765, 1.557329, 1.976363, 2.325429,
       2.604526, 2.822094, 2.993993, 0.      , 0.550532, 1.076765,
       1.557329, 1.976363, 2.325429, 2.604526, 2.822094, 2.993993,...

This error is not always reproducible. However, it has even occurred on new branches, even when the file that the results are compared to was regenerated immediately prior to running the test.

Therefore, either

  1. zeta_B is not deterministic or subject to a numerical instability (silly)
  2. Pickle is messing something up when the file is dumped
  3. Some issue from changes to this test in Move basis change to main compute function #1027 ?
@f0uriest
Copy link
Member

f0uriest commented Aug 5, 2024

Could they be the same modulo 2pi/NFP?

@YigitElma
Copy link
Collaborator

Is this test_compute_everthing? #1027 only added xyz computation to the test, it didn't change logic or etc.

@unalmis
Copy link
Collaborator Author

unalmis commented Aug 6, 2024

Could they be the same modulo 2pi/NFP?

I checked: no. This is reproducible on my machine. Repeated calls to the test give 40/60 pass,fail split respectively, sample size 8. It is consistent in that when the failure occurs it is always the same result. I'm now leaning towards option 1.

Is this test_compute_everthing? #1027 only added xyz computation to the test, it didn't change logic or etc.

Yes it looked fine when we merged. I think it was around time failure starts occurring.

@register_compute_fun(
    name="zeta_B",
    label="\\zeta_{B}",
    units="rad",
    units_long="radians",
    description="Boozer toroidal angular coordinate",
    dim=1,
    params=[],
    transforms={},
    profiles=[],
    coordinates="rtz",
    data=["zeta", "nu"],
    # default parameterization is equilibrium
)
def _zeta_B(params, transforms, profiles, data, **kwargs):
    data["zeta_B"] = data["zeta"] + data["nu"]
    return data

....
# in data_index.py
_class_inheritance = {
    ...
    "desc.magnetic_fields._core.OmnigenousField": [], # in that case shouldn't this include equilibrium or add parameteriation to above?
}

@unalmis unalmis linked a pull request Aug 7, 2024 that will close this issue
3 tasks
@unalmis unalmis added the bug Something isn't working label Aug 8, 2024
@unalmis unalmis changed the title zeta_B computation differs from master sometimes zeta_B computation is sometimes wrong Aug 8, 2024
f0uriest added a commit that referenced this issue Aug 9, 2024
#1166)

- [x] Move test compute everything to own file and exclude source grid
quantities. from commit 8a8d9de.
- [x] Remove xyz basis file in favor of correctness check to catch bug
#1088
- [x] Debug zeta_B computation. #1163 has been reproduced here (and on
master by Dario). This issue was resolved after fixing the computation
of zeta_B and theta_B for omnigenous fields.
@unalmis unalmis self-assigned this Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants