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

Allow GenericObjective and ObjectiveFromUser to work with generic "things" #1061

Merged
merged 8 commits into from
Jun 21, 2024

Conversation

ddudt
Copy link
Collaborator

@ddudt ddudt commented Jun 18, 2024

Resolves #841

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ddudt ddudt added funtionality New feature or request to do things the code can't do now. enhancement General label for enhancement. Please also tag with "Speed", "Interface", "Functionality", etc objectives Adding or improving objective functions coil stuff relating to coils and coil optimization labels Jun 18, 2024
@ddudt ddudt marked this pull request as ready for review June 19, 2024 18:57
Copy link
Contributor

github-actions bot commented Jun 19, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     +1.68 +/- 7.41     | +8.59e-03 +/- 3.78e-02 |  5.19e-01 +/- 3.3e-02  |  5.10e-01 +/- 1.8e-02  |
 test_build_transform_fft_midres         |     +1.80 +/- 2.34     | +1.06e-02 +/- 1.38e-02 |  6.01e-01 +/- 1.1e-02  |  5.90e-01 +/- 8.3e-03  |
 test_build_transform_fft_highres        |     +0.66 +/- 1.53     | +6.52e-03 +/- 1.51e-02 |  9.91e-01 +/- 1.2e-02  |  9.84e-01 +/- 8.7e-03  |
 test_equilibrium_init_lowres            |     +1.44 +/- 0.64     | +5.29e-02 +/- 2.35e-02 |  3.74e+00 +/- 1.5e-02  |  3.69e+00 +/- 1.8e-02  |
 test_equilibrium_init_medres            |     +0.81 +/- 1.09     | +3.38e-02 +/- 4.52e-02 |  4.20e+00 +/- 3.0e-02  |  4.16e+00 +/- 3.4e-02  |
 test_equilibrium_init_highres           |     +1.07 +/- 0.50     | +5.92e-02 +/- 2.78e-02 |  5.61e+00 +/- 1.9e-02  |  5.55e+00 +/- 2.1e-02  |
 test_objective_compile_dshape_current   |     +0.26 +/- 0.59     | +1.00e-02 +/- 2.28e-02 |  3.86e+00 +/- 1.7e-02  |  3.85e+00 +/- 1.5e-02  |
 test_objective_compile_atf              |     +0.19 +/- 3.04     | +1.54e-02 +/- 2.52e-01 |  8.28e+00 +/- 1.8e-01  |  8.26e+00 +/- 1.8e-01  |
 test_objective_compute_dshape_current   |     -1.10 +/- 3.56     | -1.29e-05 +/- 4.17e-05 |  1.16e-03 +/- 3.2e-05  |  1.17e-03 +/- 2.7e-05  |
 test_objective_compute_atf              |     +0.47 +/- 4.79     | +1.89e-05 +/- 1.92e-04 |  4.02e-03 +/- 1.4e-04  |  4.00e-03 +/- 1.4e-04  |
 test_objective_jac_dshape_current       |     -1.37 +/- 9.41     | -5.08e-04 +/- 3.49e-03 |  3.66e-02 +/- 2.9e-03  |  3.71e-02 +/- 1.9e-03  |
 test_objective_jac_atf                  |     -1.47 +/- 2.45     | -2.79e-02 +/- 4.66e-02 |  1.87e+00 +/- 3.7e-02  |  1.90e+00 +/- 2.8e-02  |
-test_perturb_1                          |     +1.26 +/- 0.30     | +1.64e-01 +/- 3.98e-02 |  1.32e+01 +/- 3.0e-02  |  1.31e+01 +/- 2.6e-02  |
 test_perturb_2                          |     +0.78 +/- 0.77     | +1.41e-01 +/- 1.38e-01 |  1.81e+01 +/- 9.0e-02  |  1.80e+01 +/- 1.1e-01  |
 test_proximal_jac_atf                   |     -0.28 +/- 1.34     | -2.02e-02 +/- 9.77e-02 |  7.29e+00 +/- 9.4e-02  |  7.31e+00 +/- 2.8e-02  |
 test_proximal_freeb_compute             |     +0.96 +/- 0.99     | +1.67e-03 +/- 1.74e-03 |  1.76e-01 +/- 1.5e-03  |  1.75e-01 +/- 8.6e-04  |
 test_proximal_freeb_jac                 |     +0.80 +/- 1.18     | +5.82e-02 +/- 8.60e-02 |  7.34e+00 +/- 4.7e-02  |  7.28e+00 +/- 7.2e-02  |
 test_solve_fixed_iter                   |     +0.90 +/- 7.01     | +1.33e-01 +/- 1.03e+00 |  1.49e+01 +/- 7.4e-01  |  1.48e+01 +/- 7.2e-01  |

@ddudt
Copy link
Collaborator Author

ddudt commented Jun 19, 2024

check if Optimizable or OptimizableCollection to handle different logic

desc/objectives/_generic.py Outdated Show resolved Hide resolved
desc/objectives/_generic.py Show resolved Hide resolved
desc/objectives/_generic.py Show resolved Hide resolved
Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.86%. Comparing base (caed4ce) to head (d2e9a95).
Report is 1871 commits behind head on master.

Files with missing lines Patch % Lines
desc/objectives/_generic.py 93.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1061      +/-   ##
==========================================
- Coverage   94.86%   94.86%   -0.01%     
==========================================
  Files          87       87              
  Lines       21711    21719       +8     
==========================================
+ Hits        20597    20604       +7     
- Misses       1114     1115       +1     
Files with missing lines Coverage Δ
desc/objectives/_generic.py 97.50% <93.33%> (-0.72%) ⬇️

@ddudt
Copy link
Collaborator Author

ddudt commented Jun 20, 2024

check if Optimizable or OptimizableCollection to handle different logic

I thought about how to handle general OptimizableCollections and I think it would be tricky. Because these two objectives are calling _compute on some parameterization, and collections (like a CoilSet) are not a "computable parameterization" (they have their own compute logic to handle how to compute quantities for each of their members). We could account for it if we knew the tree structure of the collection, but that logic is difficult to handle for general trees (collections within collections).

I added a check to throw a NotImplementedError if the user tries to use this on an OptimizableCollection. I think that is best for now.

@ddudt ddudt requested review from f0uriest and dpanici June 20, 2024 18:55
@ddudt ddudt merged commit f738381 into master Jun 21, 2024
17 of 18 checks passed
@ddudt ddudt deleted the dd/generic branch June 21, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coil stuff relating to coils and coil optimization enhancement General label for enhancement. Please also tag with "Speed", "Interface", "Functionality", etc funtionality New feature or request to do things the code can't do now. objectives Adding or improving objective functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow GenericObjective and ObjectiveFromUser to work with generic "things"
4 participants