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

ReduceQuadratureGrid number of radial points to match intended functionality #1188

Merged
merged 33 commits into from
Aug 27, 2024

Conversation

dpanici
Copy link
Collaborator

@dpanici dpanici commented Aug 13, 2024

  • internally to QuadratureGrid, reduces the number of radial points fro L+1 to (L+2)//2, which is the minimum amount of points to integrate a polynomial of radial degree L with jacobi weight function exactly in general

Resolves #983

@dpanici dpanici added the easy Short and simple to code or review label Aug 13, 2024
Copy link
Contributor

github-actions bot commented Aug 13, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     -4.22 +/- 11.80    | -2.14e-02 +/- 5.99e-02 |  4.86e-01 +/- 6.0e-02  |  5.08e-01 +/- 6.2e-03  |
 test_build_transform_fft_midres         |     -4.79 +/- 2.76     | -2.82e-02 +/- 1.63e-02 |  5.61e-01 +/- 1.5e-02  |  5.89e-01 +/- 5.3e-03  |
+test_build_transform_fft_highres        |     -5.84 +/- 1.58     | -5.75e-02 +/- 1.55e-02 |  9.28e-01 +/- 1.3e-02  |  9.85e-01 +/- 8.0e-03  |
 test_equilibrium_init_lowres            |     -2.90 +/- 3.91     | -1.05e-01 +/- 1.42e-01 |  3.52e+00 +/- 4.2e-02  |  3.62e+00 +/- 1.4e-01  |
 test_equilibrium_init_medres            |     -7.30 +/- 2.81     | -3.02e-01 +/- 1.16e-01 |  3.83e+00 +/- 7.3e-02  |  4.13e+00 +/- 9.0e-02  |
+test_equilibrium_init_highres           |     -9.04 +/- 2.11     | -5.06e-01 +/- 1.18e-01 |  5.10e+00 +/- 5.6e-02  |  5.60e+00 +/- 1.0e-01  |
+test_objective_compile_dshape_current   |     -6.56 +/- 2.02     | -2.53e-01 +/- 7.79e-02 |  3.61e+00 +/- 6.6e-02  |  3.86e+00 +/- 4.2e-02  |
 test_objective_compile_atf              |     -6.19 +/- 2.61     | -4.87e-01 +/- 2.05e-01 |  7.38e+00 +/- 6.3e-02  |  7.87e+00 +/- 2.0e-01  |
 test_objective_compute_dshape_current   |     -5.57 +/- 7.51     | -1.92e-04 +/- 2.59e-04 |  3.26e-03 +/- 2.6e-04  |  3.45e-03 +/- 2.6e-05  |
 test_objective_compute_atf              |     -2.25 +/- 5.46     | -2.32e-04 +/- 5.63e-04 |  1.01e-02 +/- 5.5e-04  |  1.03e-02 +/- 1.4e-04  |
 test_objective_jac_dshape_current       |     -0.23 +/- 8.01     | -8.99e-05 +/- 3.07e-03 |  3.82e-02 +/- 2.4e-03  |  3.83e-02 +/- 2.0e-03  |
 test_objective_jac_atf                  |     -5.01 +/- 4.92     | -9.61e-02 +/- 9.43e-02 |  1.82e+00 +/- 5.3e-02  |  1.92e+00 +/- 7.8e-02  |
+test_perturb_1                          |     -5.34 +/- 0.99     | -6.44e-01 +/- 1.20e-01 |  1.14e+01 +/- 7.1e-02  |  1.21e+01 +/- 9.7e-02  |
 test_perturb_2                          |     -5.51 +/- 2.15     | -9.34e-01 +/- 3.64e-01 |  1.60e+01 +/- 3.4e-01  |  1.70e+01 +/- 1.3e-01  |
-test_proximal_jac_atf                   |     +3.80 +/- 1.14     | +2.95e-01 +/- 8.87e-02 |  8.05e+00 +/- 2.6e-02  |  7.75e+00 +/- 8.5e-02  |
 test_proximal_freeb_compute             |     +4.66 +/- 3.47     | +7.98e-03 +/- 5.95e-03 |  1.79e-01 +/- 7.6e-04  |  1.71e-01 +/- 5.9e-03  |
-test_proximal_freeb_jac                 |     +3.85 +/- 1.26     | +2.72e-01 +/- 8.88e-02 |  7.33e+00 +/- 5.8e-02  |  7.06e+00 +/- 6.7e-02  |
 test_solve_fixed_iter                   |     +2.95 +/- 60.45    | +1.39e-01 +/- 2.85e+00 |  4.85e+00 +/- 2.0e+00  |  4.72e+00 +/- 2.0e+00  |

@dpanici dpanici requested review from a team and removed request for rahulgaur104, f0uriest, ddudt, kianorr, unalmis and YigitElma August 14, 2024 00:09
tests/test_bootstrap.py Show resolved Hide resolved
tests/test_grid.py Show resolved Hide resolved
tests/test_grid.py Outdated Show resolved Hide resolved
tests/test_objective_funs.py Show resolved Hide resolved
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

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

Project coverage is 95.33%. Comparing base (f9469d2) to head (7f93cea).
Report is 34 commits behind head on master.

Files Patch % Lines
desc/equilibrium/coords.py 76.92% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1188      +/-   ##
==========================================
- Coverage   95.42%   95.33%   -0.10%     
==========================================
  Files          90       90              
  Lines       22576    22581       +5     
==========================================
- Hits        21544    21527      -17     
- Misses       1032     1054      +22     
Files Coverage Δ
desc/grid.py 92.89% <100.00%> (ø)
desc/equilibrium/coords.py 83.93% <76.92%> (-7.87%) ⬇️

... and 1 file with indirect coverage changes

@dpanici dpanici marked this pull request as draft August 14, 2024 15:00
@dpanici dpanici marked this pull request as ready for review August 14, 2024 15:24
f0uriest
f0uriest previously approved these changes Aug 14, 2024
@dpanici
Copy link
Collaborator Author

dpanici commented Aug 14, 2024

Merge #1175 first

@dpanici dpanici marked this pull request as draft August 14, 2024 19:18
unalmis
unalmis previously approved these changes Aug 22, 2024
ddudt
ddudt previously approved these changes Aug 22, 2024
@unalmis unalmis dismissed stale reviews from ddudt and themself via 907b2d1 August 22, 2024 03:01
unalmis
unalmis previously approved these changes Aug 22, 2024
@unalmis
Copy link
Collaborator

unalmis commented Aug 22, 2024

My bad for making multiple commits that should have been one. I'm done committing now.

unalmis
unalmis previously approved these changes Aug 22, 2024
rahulgaur104
rahulgaur104 previously approved these changes Aug 22, 2024
)
# do surface average to get iota once
if "iota" in profiles and profiles["iota"] is None:
profiles["iota"] = eq.get_profile(["iota", "iota_r"], params=params)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is iota_r being used anywhere or is it just there so you can use Hermite?

Copy link
Collaborator

Choose a reason for hiding this comment

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

iota and iota_r get computed on QuadratureGrid then we use those values to construct Hermite spline for $\iota(\rho)$. $\iota(\rho)$ is then evaluated at whatever sequence of $\rho$ values the Newton iteration proceeds with. For $\rho, \alpha, \zeta$ coordinate mappings, the $\rho$ values don't need to change throughout Newton iteration since we can just do 1D root finding, which this PR now ensures is done.

@dpanici dpanici dismissed stale reviews from unalmis and rahulgaur104 via faeedef August 22, 2024 16:24
unalmis
unalmis previously approved these changes Aug 22, 2024
f0uriest
f0uriest previously approved these changes Aug 23, 2024
dpanici and others added 2 commits August 23, 2024 12:18
and resolve the usual compute_everything merge conflict by regenerating file
@unalmis unalmis dismissed stale reviews from f0uriest and themself via 7f93cea August 27, 2024 01:35
@dpanici dpanici merged commit fd73bdb into master Aug 27, 2024
17 of 18 checks passed
@dpanici dpanici deleted the dp/quad-grid branch August 27, 2024 14:34
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quadrature grid has too many nodes for given L, M
5 participants