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

Ku/fourier bounce part1 #1259

Merged
merged 11 commits into from
Sep 24, 2024
Merged

Ku/fourier bounce part1 #1259

merged 11 commits into from
Sep 24, 2024

Conversation

unalmis
Copy link
Collaborator

@unalmis unalmis commented Sep 22, 2024

@unalmis unalmis requested review from a team, rahulgaur104, f0uriest, ddudt, dpanici, kianorr, sinaatalay and YigitElma and removed request for a team September 22, 2024 03:31
Copy link
Contributor

github-actions bot commented Sep 22, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     -6.77 +/- 4.26     | -3.83e-02 +/- 2.41e-02 |  5.28e-01 +/- 1.7e-02  |  5.67e-01 +/- 1.7e-02  |
 test_equilibrium_init_medres            |     -3.38 +/- 7.62     | -1.48e-01 +/- 3.35e-01 |  4.25e+00 +/- 2.2e-01  |  4.39e+00 +/- 2.5e-01  |
 test_equilibrium_init_highres           |     +1.51 +/- 10.59    | +8.58e-02 +/- 6.03e-01 |  5.78e+00 +/- 4.7e-01  |  5.69e+00 +/- 3.7e-01  |
 test_objective_compile_dshape_current   |     +0.07 +/- 4.24     | +2.87e-03 +/- 1.73e-01 |  4.09e+00 +/- 1.4e-01  |  4.08e+00 +/- 9.7e-02  |
-test_objective_compute_dshape_current   |    +17.77 +/- 4.30     | +6.50e-04 +/- 1.57e-04 |  4.31e-03 +/- 1.4e-04  |  3.66e-03 +/- 7.9e-05  |
 test_objective_jac_dshape_current       |     +0.97 +/- 4.22     | +4.17e-04 +/- 1.81e-03 |  4.34e-02 +/- 1.3e-03  |  4.30e-02 +/- 1.3e-03  |
 test_perturb_2                          |     +1.42 +/- 3.51     | +2.65e-01 +/- 6.55e-01 |  1.89e+01 +/- 4.5e-01  |  1.87e+01 +/- 4.8e-01  |
 test_proximal_freeb_jac                 |     +2.13 +/- 3.03     | +1.63e-01 +/- 2.31e-01 |  7.79e+00 +/- 2.1e-01  |  7.63e+00 +/- 1.0e-01  |
 test_solve_fixed_iter                   |     +0.36 +/- 58.39    | +1.83e-02 +/- 3.00e+00 |  5.15e+00 +/- 2.1e+00  |  5.13e+00 +/- 2.1e+00  |
 test_build_transform_fft_midres         |     +5.60 +/- 6.29     | +3.55e-02 +/- 3.99e-02 |  6.69e-01 +/- 1.3e-02  |  6.33e-01 +/- 3.8e-02  |
 test_build_transform_fft_highres        |     +4.15 +/- 2.62     | +4.26e-02 +/- 2.69e-02 |  1.07e+00 +/- 1.0e-02  |  1.03e+00 +/- 2.5e-02  |
 test_equilibrium_init_lowres            |     +8.96 +/- 7.37     | +3.58e-01 +/- 2.94e-01 |  4.35e+00 +/- 7.2e-02  |  3.99e+00 +/- 2.9e-01  |
 test_objective_compile_atf              |     +5.33 +/- 3.55     | +4.26e-01 +/- 2.83e-01 |  8.41e+00 +/- 1.8e-01  |  7.98e+00 +/- 2.2e-01  |
 test_objective_compute_atf              |     +1.27 +/- 3.77     | +1.40e-04 +/- 4.17e-04 |  1.12e-02 +/- 2.5e-04  |  1.11e-02 +/- 3.3e-04  |
 test_objective_jac_atf                  |     +2.84 +/- 1.74     | +5.64e-02 +/- 3.45e-02 |  2.04e+00 +/- 2.7e-02  |  1.99e+00 +/- 2.1e-02  |
 test_perturb_1                          |     +5.83 +/- 2.78     | +7.51e-01 +/- 3.58e-01 |  1.36e+01 +/- 3.0e-01  |  1.29e+01 +/- 1.9e-01  |
 test_proximal_jac_atf                   |     +0.31 +/- 1.11     | +2.54e-02 +/- 9.14e-02 |  8.25e+00 +/- 7.5e-02  |  8.22e+00 +/- 5.3e-02  |
 test_proximal_freeb_compute             |     -0.24 +/- 1.57     | -4.48e-04 +/- 2.92e-03 |  1.85e-01 +/- 1.9e-03  |  1.85e-01 +/- 2.2e-03  |

@unalmis unalmis self-assigned this Sep 22, 2024
Copy link

codecov bot commented Sep 22, 2024

Codecov Report

Attention: Patch coverage is 95.16129% with 3 lines in your changes missing coverage. Please review.

Project coverage is 95.43%. Comparing base (89e7fe3) to head (41b6ec8).
Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
desc/integrals/bounce_utils.py 91.42% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1259       +/-   ##
===========================================
+ Coverage   59.59%   95.43%   +35.84%     
===========================================
  Files          95       95               
  Lines       23425    23440       +15     
===========================================
+ Hits        13961    22371     +8410     
+ Misses       9464     1069     -8395     
Files with missing lines Coverage Δ
desc/backend.py 90.18% <100.00%> (+1.29%) ⬆️
desc/equilibrium/coords.py 88.38% <100.00%> (+25.98%) ⬆️
desc/integrals/bounce_integral.py 100.00% <100.00%> (+32.69%) ⬆️
desc/integrals/interp_utils.py 100.00% <100.00%> (ø)
desc/integrals/quad_utils.py 100.00% <100.00%> (+8.10%) ⬆️
desc/integrals/bounce_utils.py 93.19% <91.42%> (+12.18%) ⬆️

... and 72 files with indirect coverage changes

@unalmis unalmis added the override codecov Override codecov label Sep 22, 2024
@dpanici
Copy link
Collaborator

dpanici commented Sep 23, 2024

Clever of you to put the fix to #1260 here so I would have to review the rest of the PR as well

@unalmis unalmis added the easy Short and simple to code or review label Sep 23, 2024
@@ -190,14 +184,67 @@ def leggauss_lob(deg, interior_only=False):
return x, w


def chebgauss_uniform(deg):
"""Gauss-Chebyshev quadrature with uniformly spaced nodes.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I totally understand this. The points are linearly spaced, and the weights are uniform, so isn't this just 1st order reimann sum?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes assuming the riemann sum starts and ends at the correct endpoints, it is the same as gaussian quadrature in a transformed variable y

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a longer comment on the ripple branch, but put another way simply, all quadrature tricks can be stated in the most general sense to make your function as smooth as possible before integrating. This ensures that a sample of the integrand at one point is the best approximation to stuff around it. Hence equivalently you can increase observed smoothness by putting more nodes in non-smooth areas of the function.

Secondary to the smoothness objective is the objective to minimize interpolation errors that result from weaknesses in the choice of basis functions. A Gaussian quadrature in x = 1/pitch sacrifices the first goal, for the secondary goal: minimizing runge. If the quantity to integrate is sufficiently smooth to begin with, then this choice is fine. Neither Gamma_c nor epsilon effective were sufficiently smooth for this. For the former it's easy to see from the expression, for the latter I can only claim that's what i see empirically

dpanici
dpanici previously approved these changes Sep 23, 2024
dpanici
dpanici previously approved these changes Sep 24, 2024
@unalmis unalmis added override codecov Override codecov and removed override codecov Override codecov labels Sep 24, 2024
dpanici
dpanici previously approved these changes Sep 24, 2024
@unalmis
Copy link
Collaborator Author

unalmis commented Sep 24, 2024

feel free to merge whenever; codecov might block me

@dpanici dpanici merged commit 7d9ce27 into master Sep 24, 2024
24 checks passed
@dpanici dpanici deleted the ku/fourier_bounce_part1 branch September 24, 2024 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy Short and simple to code or review override codecov Override codecov
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_compute_everything fails
4 participants