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

Remaining bugs with duplicate nodes, add grid developer guide #409

Merged
merged 11 commits into from
Feb 8, 2023

Conversation

unalmis
Copy link
Collaborator

@unalmis unalmis commented Jan 17, 2023

  • Squash bugs
    A list of bug fixes follows. The use case that would uncover these bugs is rather narrow, which is why it wasn't detected by previous tests, and is unlikely to affect most stuff. All of these bugs involve duplicate nodes, however, none of them were introduced by Duplicate node spacing #386 . This PR just finds bugs that were missed by that one.

    • Fixed issue where theta and zeta surface integrals on grids with duplicate node at 0 and 2 pi (or 2 pi/NFP) (e.g. when endpoint=True) computed a value that was off by a factor of 2 only on the surface at 0 and 2 pi. All other surfaces were computed correctly.
      In other words, 2 elements of the returned array of length grid.num_surface_label were off by factor 2.
    • More massaging of inputs so that the grid class works with weird inputs. This is required for correctness of some of the algorithms, but unlikely to be the cause of any bugs as a runtime error would have been raised immediately.
    • Added logic so LinearGrids with duplicate nodes and symmetry have correct weights and spacing.
    • If the user supplies an incorrect marker for endpoint, for example by providing endpoint=False to the LinearGrid constructor while also providing arrays with nodes that include a duplicate endpoint, the weights would be computed incorrectly. This was fixed so that the correct weights are computed regardless of the user's tricks.
  • Add bug catchers

    • many more tests
  • Add bug repellent

    • add walk-through of grid.py to developer guide explaining everything with nice visuals

A list of bugs fixes follows. Note that the use case is rather narrow,
which is why it wasn't detected by previous tests, and is unlikely
to affect most stuff.

- Fixed issue where theta and zeta surface integrals on grids with
  duplicate node at 0 and 2 pi (or 2 pi/NFP) (e.g. when endpoint=True)
  computed a value that was off by a factor of 2 only on the surface
  at 0 and 2 pi. All other surfaces were computed correctly. In other
  words, 2 elements of the returned array of length
  grid.num_surface_label were off by factor 2.
- More massaging of inputs so that the grid class works with weird
  inputs. This is required for correctness of some of the
  algorithms, but unlikely to be the cause of any bugs
  as a runtime error would have been raised immediately.
- Added logic so LinearGrids with duplicate nodes and symmetry have
  correct weights and spacing.
- If the user supplies an incorrect marker for endpoint, for example
  by providing endpoint=False to the LinearGrid constructor while
  also providing arrays with nodes that include a duplicate endpoint,
  the weights would be computed incorrectly. This was fixed so that
  the correct weights are computed  regardless of the user's trickery.
- Many more tests.
@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Base: 93.59% // Head: 93.60% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (11ccf74) compared to base (128ce00).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #409   +/-   ##
=======================================
  Coverage   93.59%   93.60%           
=======================================
  Files          68       68           
  Lines       14039    14055   +16     
=======================================
+ Hits        13140    13156   +16     
  Misses        899      899           
Impacted Files Coverage Δ
desc/compute/utils.py 97.15% <100.00%> (+0.08%) ⬆️
desc/grid.py 98.15% <100.00%> (+0.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -0,0 +1,1225 @@
{
Copy link
Member

@f0uriest f0uriest Feb 6, 2023

Choose a reason for hiding this comment

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

Shouldn't the rho integrals should be $\int_0^1 \rho d\rho$?


Reply via ReviewNB

Copy link
Collaborator Author

@unalmis unalmis Feb 6, 2023

Choose a reason for hiding this comment

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

Do you mean that in the sense that $\rho$ is the area Jacobian for basic polar geometries, with $dA = \rho d\rho d\theta$?

If we wanted to compute the actual area of a $\zeta$ surface, we would weight it by the area Jacobian for that surface in our geometry: $\int_0^1 \int_0^{2\pi} \lvert \ e_{\rho} \times e_{\theta} \rvert d\rho d\theta$

When I mention "node area" in this document I am just referring to the product of the differential elements in the columns of grid.spacing for the row associated with that node. For a $\zeta$ surface the unweighted area, which is the sum of these products over all the nodes on the surface, would be $\int_0^1 \int_0^{2\pi} d\rho d\theta = 2\pi$.

I think that is an invariant the grid should try to keep. That way when we supply a Jacobian factor in the integral, whether that be for an area or volume, we know that the integral covers the entire domain.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I was thinking of it like the area in the computational domain which in this case is a disk, so the area should be pi. But I can see your point as well about keeping the Jacobian separate. I'm fine with it if everyone else is.

docs/notebooks/dev_guide/grid.ipynb Show resolved Hide resolved
f0uriest
f0uriest previously approved these changes Feb 7, 2023
ddudt
ddudt previously approved these changes Feb 7, 2023
desc/compute/utils.py Outdated Show resolved Hide resolved
tests/test_grid.py Outdated Show resolved Hide resolved
f0uriest
f0uriest previously approved these changes Feb 7, 2023
- fix markdown headings so that every section does not show up
  as a new website on the documentation page
- clarify why duplicate node algorithm is correct
- document that line integrals overweight duplicate nodes by
  square root of the duplicity.
  The easy fix is documented in the grid docs.
  But it requires a new grid.spacing.
@unalmis unalmis dismissed stale reviews from f0uriest and ddudt via a36ab27 February 8, 2023 06:32
@unalmis unalmis requested review from f0uriest and ddudt February 8, 2023 15:44
@unalmis unalmis merged commit 796de3c into master Feb 8, 2023
@unalmis unalmis deleted the grid_doc branch February 8, 2023 20:20
@unalmis unalmis added the bug fix Something was fixed label Jul 22, 2024
@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 fix Something was fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants