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

Document basis for input data #1124

Merged
merged 3 commits into from
Jul 12, 2024
Merged

Document basis for input data #1124

merged 3 commits into from
Jul 12, 2024

Conversation

unalmis
Copy link
Collaborator

@unalmis unalmis commented Jul 11, 2024

No description provided.

@unalmis unalmis added the documentation Add documentation or better warnings etc. label Jul 11, 2024
@unalmis
Copy link
Collaborator Author

unalmis commented Jul 11, 2024

feel free to merge after approval

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.24%. Comparing base (fc06523) to head (1c6e191).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1124   +/-   ##
=======================================
  Coverage   95.24%   95.24%           
=======================================
  Files          87       87           
  Lines       21659    21659           
=======================================
  Hits        20630    20630           
  Misses       1029     1029           
Files Coverage Δ
desc/compute/utils.py 96.36% <ø> (ø)
desc/equilibrium/equilibrium.py 96.30% <ø> (ø)
desc/geometry/core.py 95.91% <ø> (ø)
desc/geometry/curve.py 93.10% <ø> (ø)

Copy link
Contributor

github-actions bot commented Jul 12, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     -0.98 +/- 8.07     | -5.06e-03 +/- 4.19e-02 |  5.14e-01 +/- 3.4e-02  |  5.19e-01 +/- 2.4e-02  |
 test_build_transform_fft_midres         |     -0.97 +/- 2.66     | -5.84e-03 +/- 1.60e-02 |  5.96e-01 +/- 1.3e-02  |  6.02e-01 +/- 9.2e-03  |
 test_build_transform_fft_highres        |     -0.33 +/- 3.81     | -3.28e-03 +/- 3.76e-02 |  9.84e-01 +/- 2.4e-02  |  9.88e-01 +/- 2.9e-02  |
 test_equilibrium_init_lowres            |     +0.40 +/- 1.23     | +1.49e-02 +/- 4.50e-02 |  3.69e+00 +/- 2.9e-02  |  3.67e+00 +/- 3.4e-02  |
 test_equilibrium_init_medres            |     -3.79 +/- 6.95     | -1.64e-01 +/- 3.01e-01 |  4.16e+00 +/- 1.1e-01  |  4.33e+00 +/- 2.8e-01  |
+test_equilibrium_init_highres           |     -3.43 +/- 1.03     | -2.04e-01 +/- 6.11e-02 |  5.75e+00 +/- 5.4e-02  |  5.95e+00 +/- 2.8e-02  |
 test_objective_compile_dshape_current   |     -2.11 +/- 1.23     | -8.37e-02 +/- 4.86e-02 |  3.88e+00 +/- 2.8e-02  |  3.96e+00 +/- 4.0e-02  |
 test_objective_compile_atf              |     -2.53 +/- 2.17     | -2.17e-01 +/- 1.86e-01 |  8.36e+00 +/- 8.4e-02  |  8.58e+00 +/- 1.7e-01  |
 test_objective_compute_dshape_current   |     -1.54 +/- 5.21     | -1.99e-05 +/- 6.72e-05 |  1.27e-03 +/- 3.9e-05  |  1.29e-03 +/- 5.5e-05  |
 test_objective_compute_atf              |     -7.18 +/- 5.80     | -3.35e-04 +/- 2.71e-04 |  4.34e-03 +/- 1.5e-04  |  4.67e-03 +/- 2.3e-04  |
 test_objective_jac_dshape_current       |     -2.70 +/- 6.57     | -1.08e-03 +/- 2.63e-03 |  3.89e-02 +/- 2.5e-03  |  4.00e-02 +/- 8.2e-04  |
 test_objective_jac_atf                  |     +0.30 +/- 2.85     | +5.70e-03 +/- 5.35e-02 |  1.89e+00 +/- 3.0e-02  |  1.88e+00 +/- 4.4e-02  |
 test_perturb_1                          |     +0.40 +/- 2.63     | +5.23e-02 +/- 3.47e-01 |  1.32e+01 +/- 1.4e-01  |  1.32e+01 +/- 3.2e-01  |
 test_perturb_2                          |     +0.84 +/- 1.08     | +1.51e-01 +/- 1.96e-01 |  1.82e+01 +/- 1.6e-01  |  1.81e+01 +/- 1.1e-01  |
 test_proximal_jac_atf                   |     -0.15 +/- 1.63     | -1.12e-02 +/- 1.19e-01 |  7.31e+00 +/- 9.4e-02  |  7.33e+00 +/- 7.4e-02  |
 test_proximal_freeb_compute             |     -0.67 +/- 1.15     | -1.19e-03 +/- 2.06e-03 |  1.78e-01 +/- 1.7e-03  |  1.79e-01 +/- 1.2e-03  |
 test_proximal_freeb_jac                 |     +1.27 +/- 1.96     | +9.36e-02 +/- 1.45e-01 |  7.48e+00 +/- 1.3e-01  |  7.39e+00 +/- 7.1e-02  |
 test_solve_fixed_iter                   |     -0.22 +/- 8.90     | -3.25e-02 +/- 1.34e+00 |  1.50e+01 +/- 1.0e+00  |  1.51e+01 +/- 8.8e-01  |

@ddudt ddudt requested review from ddudt and removed request for daniel-dudt July 12, 2024 01:08
@@ -121,6 +123,9 @@ def _compute(
):
"""Same as above but without checking inputs for faster recursion.

All vectors v = [vᴿ, v^ϕ, vᶻ] in ``data`` should be given in contravariant
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get the phi superscript to work?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also in all the other new docstrings

Copy link
Member

Choose a reason for hiding this comment

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

I don't think unicode has a superscript phi.

Another small nit: cylindrical coordinates are orthogonal, so the co- and contra-variant bases are the same

Copy link
Collaborator Author

@unalmis unalmis Jul 12, 2024

Choose a reason for hiding this comment

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

I couldn't find superscript phi either.

Contravariant = covariant only if the coordinate system is orthonormal. Orthogonal is not sufficient as that only implies direction of basis vectors are same and not their magnitudes. (I think for cylindrical the phi component will differ by factors of R).

Copy link
Member

Choose a reason for hiding this comment

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

hmm i think you're right, nvm

Copy link
Collaborator

Choose a reason for hiding this comment

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

On the topic of our cylindrical coordinates not being orthonormal, does that cause any problems when we take cross-products in those coordinates with our cross util function? I'm trying to track down some more bugs with the Frenet-Serret frame and was wondering about this

Copy link
Collaborator Author

@unalmis unalmis Jul 12, 2024

Choose a reason for hiding this comment

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

Our cross and dot product utilities only work in orthonormal (and right-handed for cross) coordinate systems, so they would not work in cylindrical coordinates which are orthogonal but not orthonormal.

However, I now recall that the tuple of values we return when we request a vector quantity is neither the contravariant components $[v^R, v^\phi, v^Z]$ nor the covariant components $[v_R, v_\phi, v_Z]$ of the cylindrical coordinate system $R, \phi, Z$.

Instead, we return the coordinates $[v^1, v^2, v^3]$ of a different, orthonormal, coordinate system.

For a vector $v$ we have
$v = v^R e_R + v^\phi e_\phi + v^Z e_Z = v_R e^R + v_\phi e^\phi + v_Z e^Z = v^1 \hat{R} + v^2 \hat{\phi} + v^3 \hat{Z}$.

For $R, \phi, Z$ coordinates, all three sets of basis vectors point in the same direction but the magntiudes obey $\Vert e_\phi \Vert = \Vert v^1 \hat{\phi} \Vert$ and $\Vert e^\phi \Vert = \Vert (1/v^1) \hat{\phi} \Vert $. Hence, the $\phi$ components will differ by the inverse of these factors.

Books like dhaeseleer's and frankel's will refer to components of a vector in the cylindrical coordinate system $R, \phi, Z$ as $[v^R, v^\phi, v^Z]$, while most of wikipedia will use $[v^1, v^2, v^3]$. So you may want to check whether this discrepancy in the $\phi$ component of the vector is causing issues.

Copy link
Collaborator Author

@unalmis unalmis Jul 12, 2024

Choose a reason for hiding this comment

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

As an example of how this could cause issues, note that here https://en.wikipedia.org/wiki/Gradient#Gradient_of_a_vector_field, the quantity $\partial f^i / \partial x_j$ in $\nabla f$ is differentiating the contravariant components $f^R, f^\phi, f^Z$ with respect to $R, \phi, Z$ and not the components $f^1, f^2, f^3$. You would miss a product rule on the $R$ and $\phi$ derivatives if you use the wrong one.

f0uriest
f0uriest previously approved these changes Jul 12, 2024
@unalmis unalmis merged commit ac10970 into master Jul 12, 2024
18 checks passed
@unalmis unalmis deleted the compute_document branch July 12, 2024 14:54
@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
documentation Add documentation or better warnings etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants