Make integration stuff visible to users and update dev guide #1212
Conversation
There was a problem hiding this comment.
We'll have a bounce_integral file in this folder as well.
| # TODO: Make the surface integral stuff objects with a callable method instead of | ||
| # returning functions. Would make simpler, allow users to actually see the | ||
| # docstrings of the methods, and less bookkeeping to default to more | ||
| # efficient methods on tensor product grids. |
There was a problem hiding this comment.
I know JAX style is functional programming, but i think OOP is better
There was a problem hiding this comment.
yeah I've been seeing similar things on #1043. Probably less of an issue here since the local functions are usually created under jit themselves so they won't cause a lot of recompilation but classes with jitted methods also work well.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1212 +/- ##
==========================================
- Coverage 95.44% 95.42% -0.03%
==========================================
Files 87 89 +2
Lines 22423 22434 +11
==========================================
+ Hits 21401 21407 +6
- Misses 1022 1027 +5
|
| benchmark_name | dt(%) | dt(s) | t_new(s) | t_old(s) |
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
test_build_transform_fft_lowres | +4.59 +/- 6.03 | +2.37e-02 +/- 3.11e-02 | 5.39e-01 +/- 2.7e-02 | 5.15e-01 +/- 1.6e-02 |
test_build_transform_fft_midres | +3.63 +/- 5.58 | +2.18e-02 +/- 3.35e-02 | 6.21e-01 +/- 3.2e-02 | 5.99e-01 +/- 8.4e-03 |
test_build_transform_fft_highres | +1.32 +/- 2.20 | +1.31e-02 +/- 2.18e-02 | 1.00e+00 +/- 1.9e-02 | 9.90e-01 +/- 1.0e-02 |
test_equilibrium_init_lowres | +3.46 +/- 8.21 | +1.29e-01 +/- 3.05e-01 | 3.85e+00 +/- 2.8e-01 | 3.72e+00 +/- 1.2e-01 |
test_equilibrium_init_medres | +2.58 +/- 1.42 | +1.07e-01 +/- 5.93e-02 | 4.27e+00 +/- 5.7e-02 | 4.16e+00 +/- 1.5e-02 |
test_equilibrium_init_highres | +5.20 +/- 5.96 | +2.89e-01 +/- 3.32e-01 | 5.86e+00 +/- 3.3e-01 | 5.57e+00 +/- 3.8e-02 |
test_objective_compile_dshape_current | +0.37 +/- 2.41 | +1.43e-02 +/- 9.25e-02 | 3.85e+00 +/- 9.2e-02 | 3.84e+00 +/- 8.7e-03 |
test_objective_compile_atf | +0.85 +/- 1.67 | +6.67e-02 +/- 1.31e-01 | 7.93e+00 +/- 1.2e-01 | 7.86e+00 +/- 4.7e-02 |
test_objective_compute_dshape_current | +1.55 +/- 2.37 | +5.41e-05 +/- 8.29e-05 | 3.55e-03 +/- 5.3e-05 | 3.50e-03 +/- 6.4e-05 |
test_objective_compute_atf | +1.97 +/- 2.37 | +2.04e-04 +/- 2.45e-04 | 1.05e-02 +/- 2.1e-04 | 1.03e-02 +/- 1.2e-04 |
test_objective_jac_dshape_current | -0.72 +/- 7.98 | -2.89e-04 +/- 3.19e-03 | 3.98e-02 +/- 2.2e-03 | 4.00e-02 +/- 2.3e-03 |
test_objective_jac_atf | -0.15 +/- 3.49 | -2.81e-03 +/- 6.47e-02 | 1.85e+00 +/- 5.6e-02 | 1.86e+00 +/- 3.3e-02 |
test_perturb_1 | +4.32 +/- 1.93 | +5.26e-01 +/- 2.35e-01 | 1.27e+01 +/- 2.2e-01 | 1.22e+01 +/- 9.0e-02 |
test_perturb_2 | +3.14 +/- 1.83 | +5.42e-01 +/- 3.16e-01 | 1.78e+01 +/- 2.6e-01 | 1.73e+01 +/- 1.8e-01 |
test_proximal_jac_atf | +0.63 +/- 1.08 | +5.12e-02 +/- 8.74e-02 | 8.17e+00 +/- 8.0e-02 | 8.12e+00 +/- 3.5e-02 |
test_proximal_freeb_compute | +2.13 +/- 1.10 | +3.88e-03 +/- 2.02e-03 | 1.86e-01 +/- 1.3e-03 | 1.82e-01 +/- 1.5e-03 |
test_proximal_freeb_jac | +2.82 +/- 6.24 | +2.11e-01 +/- 4.66e-01 | 7.68e+00 +/- 4.6e-01 | 7.47e+00 +/- 4.5e-02 |
test_solve_fixed_iter | +0.87 +/- 57.87 | +4.35e-02 +/- 2.90e+00 | 5.06e+00 +/- 2.1e+00 | 5.02e+00 +/- 2.0e+00 | |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
This tutorial is thorough and existed when the issue was created. Does this resolve the issue? |
unalmis
changed the title
unalmis
changed the title
unalmis
changed the title
unalmis
requested review from
a team,
rahulgaur104,
f0uriest,
ddudt,
dpanici,
kianorr,
sinaatalay and
YigitElma
and removed request for
a team
There was a problem hiding this comment.
We could also add an __init__.py to integrals and import stuff there to make it more uniform.
IE,
from desc.integrals import compute_B_plasma, surface_integralsrather than
from desc.integrals.surface_integrals import surface_integral
from desc.integrals.singularities import compute_B_plasma
|
I will have a couple of suggestions for the grid notebook, but there is no easy way to put comments on the code. Instead, I will try to write them here. Can you wait for tonight? @unalmis Also, I will just look at the notebook, so some of the things might be from the the previous version :( |
Might be easier to write them here: https://app.reviewnb.com/PlasmaControl/DESC/blob/integrals/docs/notebooks/dev_guide/grid.ipynb/ |
Oh thanks, it's a lot better :D |
|
Ok, I added some comments. I guess none of them is regarding your changes but the old version itself. You can either make changes or I can do them on another PR, it is up to you @unalmis |
|
@YigitElma Feel free to make any changes, thanks. It's fine to do it here, or we can merge this and do it in the dev guide pull request later. |
This PR moves the integration utilities to their own subfolder so that they are visible to users. We have decided not to include in public API, because that's for optimization stuff.
surface_integrals.py.grid.pydeveloper guide.