-
Notifications
You must be signed in to change notification settings - Fork 30
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
Explain to users why variable computations may differ #1615
Conversation
| benchmark_name | dt(%) | dt(s) | t_new(s) | t_old(s) |
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
test_build_transform_fft_lowres | +7.02 +/- 5.03 | +4.24e-02 +/- 3.03e-02 | 6.45e-01 +/- 2.7e-02 | 6.03e-01 +/- 1.3e-02 |
test_equilibrium_init_medres | +10.48 +/- 4.06 | +4.40e-01 +/- 1.70e-01 | 4.64e+00 +/- 1.5e-01 | 4.20e+00 +/- 7.1e-02 |
test_equilibrium_init_highres | +3.40 +/- 4.08 | +1.77e-01 +/- 2.12e-01 | 5.37e+00 +/- 1.8e-01 | 5.20e+00 +/- 1.1e-01 |
test_objective_compile_dshape_current | +0.21 +/- 5.12 | +8.86e-03 +/- 2.14e-01 | 4.18e+00 +/- 1.4e-01 | 4.18e+00 +/- 1.6e-01 |
test_objective_compute_dshape_current | +0.84 +/- 1.95 | +4.52e-05 +/- 1.05e-04 | 5.45e-03 +/- 9.5e-05 | 5.40e-03 +/- 4.5e-05 |
test_objective_jac_dshape_current | +0.08 +/- 6.36 | +3.41e-05 +/- 2.75e-03 | 4.32e-02 +/- 1.9e-03 | 4.31e-02 +/- 2.0e-03 |
test_perturb_2 | -0.95 +/- 3.52 | -1.97e-01 +/- 7.28e-01 | 2.05e+01 +/- 4.5e-01 | 2.07e+01 +/- 5.7e-01 |
test_proximal_jac_atf_with_eq_update | +1.10 +/- 0.79 | +1.87e-01 +/- 1.35e-01 | 1.72e+01 +/- 1.0e-01 | 1.70e+01 +/- 8.8e-02 |
test_proximal_freeb_jac | -2.58 +/- 2.23 | -1.32e-01 +/- 1.14e-01 | 4.98e+00 +/- 1.1e-01 | 5.11e+00 +/- 4.1e-02 |
test_solve_fixed_iter_compiled | -3.08 +/- 3.32 | -6.65e-01 +/- 7.16e-01 | 2.09e+01 +/- 2.4e-01 | 2.16e+01 +/- 6.8e-01 |
test_LinearConstraintProjection_build | -0.77 +/- 2.31 | -8.54e-02 +/- 2.57e-01 | 1.10e+01 +/- 2.0e-01 | 1.11e+01 +/- 1.6e-01 |
test_objective_compute_ripple_spline | +0.08 +/- 4.12 | +2.77e-04 +/- 1.42e-02 | 3.45e-01 +/- 7.7e-03 | 3.45e-01 +/- 1.2e-02 |
test_objective_grad_ripple_spline | +2.09 +/- 2.06 | +2.96e-02 +/- 2.91e-02 | 1.44e+00 +/- 2.8e-02 | 1.41e+00 +/- 9.4e-03 |
test_build_transform_fft_midres | +0.17 +/- 7.82 | +1.05e-03 +/- 4.86e-02 | 6.23e-01 +/- 4.7e-02 | 6.22e-01 +/- 1.3e-02 |
test_build_transform_fft_highres | +0.37 +/- 3.24 | +3.38e-03 +/- 2.92e-02 | 9.06e-01 +/- 2.9e-02 | 9.03e-01 +/- 3.6e-03 |
test_equilibrium_init_lowres | -1.35 +/- 4.26 | -5.28e-02 +/- 1.67e-01 | 3.87e+00 +/- 8.8e-02 | 3.92e+00 +/- 1.4e-01 |
test_objective_compile_atf | -2.87 +/- 2.33 | -2.40e-01 +/- 1.95e-01 | 8.10e+00 +/- 1.4e-01 | 8.34e+00 +/- 1.3e-01 |
test_objective_compute_atf | -3.78 +/- 4.15 | -6.27e-04 +/- 6.89e-04 | 1.60e-02 +/- 1.7e-04 | 1.66e-02 +/- 6.7e-04 |
test_objective_jac_atf | -1.40 +/- 2.50 | -2.74e-02 +/- 4.89e-02 | 1.93e+00 +/- 4.4e-02 | 1.96e+00 +/- 2.2e-02 |
test_perturb_1 | -1.33 +/- 2.24 | -1.99e-01 +/- 3.34e-01 | 1.47e+01 +/- 2.7e-01 | 1.49e+01 +/- 2.0e-01 |
test_proximal_jac_atf | -0.32 +/- 0.95 | -2.60e-02 +/- 7.64e-02 | 8.03e+00 +/- 6.1e-02 | 8.06e+00 +/- 4.6e-02 |
test_proximal_freeb_compute | +1.32 +/- 1.14 | +2.48e-03 +/- 2.15e-03 | 1.90e-01 +/- 1.7e-03 | 1.88e-01 +/- 1.3e-03 |
test_solve_fixed_iter | -0.07 +/- 2.96 | -2.21e-02 +/- 9.55e-01 | 3.22e+01 +/- 8.4e-01 | 3.23e+01 +/- 4.5e-01 |
test_objective_compute_ripple | +0.11 +/- 2.09 | +7.96e-04 +/- 1.45e-02 | 6.96e-01 +/- 6.7e-03 | 6.95e-01 +/- 1.3e-02 |
test_objective_grad_ripple | +0.57 +/- 1.10 | +1.50e-02 +/- 2.87e-02 | 2.64e+00 +/- 2.1e-02 | 2.62e+00 +/- 2.0e-02 | |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1615 +/- ##
=======================================
Coverage 95.72% 95.72%
=======================================
Files 101 101
Lines 26356 26356
=======================================
Hits 25228 25228
Misses 1128 1128
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for #1614 s sake, making this kind of clarification to eq.compute
s grid
docs would be nice, too. Currently, it states,
grid : Grid, optional
Grid of coordinates to evaluate at. Defaults to the quadrature grid.
maybe something like,
grid : Grid, optional
Grid of coordinates to evaluate at. Defaults to the quadrature grid. Note that this is the grid that final output values belong to, but not the grid to use during computing the quantities. To use this grid for the compute functions, set `override_grid=False`
This is discussed in the override grid parameter docstring for eq.compute
I see the docs, but I still think it can be clearer this way |
I do not think there is anything confusing with the override grid documentation, so I do not feel obligated to duplicate it ten lines above. As a general principle the more writing that is added, the less people will read it |
I think most people just read the first 2 3 arguments docs and not the rest, since they are optional. In this case, this detail is stated in the optional part. But anyway |
Resolves #1424 (#1424 (comment)) and resolves #1614.