-
Notifications
You must be signed in to change notification settings - Fork 26
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
Remove numpy backend #1150
base: master
Are you sure you want to change the base?
Remove numpy backend #1150
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1150 +/- ##
==========================================
+ Coverage 95.59% 95.65% +0.05%
==========================================
Files 96 96
Lines 24601 24588 -13
==========================================
+ Hits 23518 23519 +1
+ Misses 1083 1069 -14
|
| benchmark_name | dt(%) | dt(s) | t_new(s) | t_old(s) |
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
test_build_transform_fft_midres | +1.46 +/- 3.24 | +8.90e-03 +/- 1.98e-02 | 6.18e-01 +/- 1.7e-02 | 6.09e-01 +/- 9.2e-03 |
test_build_transform_fft_highres | +0.04 +/- 2.18 | +4.22e-04 +/- 2.14e-02 | 9.80e-01 +/- 1.7e-02 | 9.80e-01 +/- 1.3e-02 |
test_equilibrium_init_lowres | +0.45 +/- 3.05 | +1.80e-02 +/- 1.21e-01 | 3.97e+00 +/- 7.8e-02 | 3.96e+00 +/- 9.2e-02 |
test_objective_compile_atf | +0.70 +/- 4.77 | +5.53e-02 +/- 3.78e-01 | 7.97e+00 +/- 2.8e-01 | 7.92e+00 +/- 2.6e-01 |
test_objective_compute_atf | +0.43 +/- 5.11 | +4.63e-05 +/- 5.52e-04 | 1.08e-02 +/- 4.7e-04 | 1.08e-02 +/- 2.9e-04 |
test_objective_jac_atf | -1.01 +/- 2.81 | -1.93e-02 +/- 5.39e-02 | 1.90e+00 +/- 4.6e-02 | 1.92e+00 +/- 2.8e-02 |
test_perturb_1 | +0.25 +/- 1.62 | +3.61e-02 +/- 2.35e-01 | 1.46e+01 +/- 7.5e-02 | 1.45e+01 +/- 2.2e-01 |
test_proximal_jac_atf | -0.48 +/- 0.48 | -3.89e-02 +/- 3.88e-02 | 8.08e+00 +/- 3.6e-02 | 8.12e+00 +/- 1.5e-02 |
test_proximal_freeb_compute | +0.10 +/- 0.80 | +2.03e-04 +/- 1.58e-03 | 1.98e-01 +/- 1.1e-03 | 1.98e-01 +/- 1.2e-03 |
test_solve_fixed_iter_compiled | -0.25 +/- 1.99 | -4.17e-02 +/- 3.35e-01 | 1.68e+01 +/- 2.5e-01 | 1.68e+01 +/- 2.2e-01 |
test_build_transform_fft_lowres | +0.62 +/- 6.06 | +3.17e-03 +/- 3.10e-02 | 5.14e-01 +/- 3.0e-02 | 5.11e-01 +/- 8.8e-03 |
test_equilibrium_init_medres | +6.27 +/- 2.76 | +2.57e-01 +/- 1.13e-01 | 4.36e+00 +/- 5.3e-02 | 4.10e+00 +/- 1.0e-01 |
test_equilibrium_init_highres | +4.12 +/- 6.28 | +2.20e-01 +/- 3.35e-01 | 5.56e+00 +/- 3.2e-01 | 5.34e+00 +/- 1.0e-01 |
test_objective_compile_dshape_current | -0.15 +/- 6.32 | -5.84e-03 +/- 2.41e-01 | 3.82e+00 +/- 2.4e-01 | 3.82e+00 +/- 4.3e-02 |
test_objective_compute_dshape_current | +0.94 +/- 2.00 | +3.40e-05 +/- 7.27e-05 | 3.66e-03 +/- 6.9e-05 | 3.63e-03 +/- 2.2e-05 |
test_objective_jac_dshape_current | +4.77 +/- 9.65 | +1.91e-03 +/- 3.87e-03 | 4.20e-02 +/- 2.4e-03 | 4.01e-02 +/- 3.0e-03 |
test_perturb_2 | +3.55 +/- 4.04 | +6.75e-01 +/- 7.67e-01 | 1.97e+01 +/- 3.8e-01 | 1.90e+01 +/- 6.7e-01 |
test_proximal_freeb_jac | -0.90 +/- 1.68 | -6.76e-02 +/- 1.25e-01 | 7.40e+00 +/- 1.1e-01 | 7.47e+00 +/- 5.6e-02 |
test_solve_fixed_iter | -0.91 +/- 3.17 | -2.58e-01 +/- 8.98e-01 | 2.81e+01 +/- 3.4e-01 | 2.83e+01 +/- 8.3e-01 |
test_LinearConstraintProjection_build | -0.95 +/- 2.16 | -2.17e-01 +/- 4.92e-01 | 2.26e+01 +/- 3.9e-01 | 2.28e+01 +/- 3.0e-01 | |
This will cause merge conflicts with all my open PRs, so could we wait to merge this? |
It's fine for me. I just opened this PR to close an easy issue. |
run tests on numpy backend and see what fails? Although wait for the |
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.
Does any part of the code actually work without JAX? If not, we should just throw an error instead of a warning.
"DESC version {}, using numpy backend, version={}, dtype={}".format( | ||
desc.__version__, np.__version__, np.linspace(0, 1).dtype | ||
) | ||
warnings.warn( |
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.
should raise a DeprecationWarning
print( | ||
"DESC version {}, using NumPy backend, version={}, dtype={}".format( | ||
desc.__version__, np.__version__, y.dtype | ||
warnings.warn( |
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.
should raise a DeprecationWarning
""" | ||
out = scipy.optimize.root_scalar( | ||
fun, args, x0=x0, fprime=jac, xtol=tol, rtol=tol | ||
warnings.warn( |
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.
should raise a DeprecationWarning
@@ -73,6 +73,7 @@ ignore = | |||
per-file-ignores = | |||
# need to import things to top level even if they aren't used there | |||
desc/*/__init__.py: F401 | |||
desc/backend.py: F401 |
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.
Why is this needed?
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.
For some reason, flake was complaining about imports not being used.
@unalmis would this still cause lots of conflicts if merged? |
it would cause one that looks simple to resolve |
Resolves #1149