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

Throw error for when Curve is used for coil objectives #1141

Merged
merged 18 commits into from
Aug 28, 2024
Merged

Conversation

kianorr
Copy link
Collaborator

@kianorr kianorr commented Jul 23, 2024

Resolves #1020

The given example in #1020 uses a FourierPlanarCurve which actually results in a list of numpy arrays being created after going through tree_flatten. This PR gives a better error message, but I'm not sure if we can explicitly say that a Curve was inputted.

If there is a Curve object in a CoilSet then there are already checks in the CoilSet class that throw an error.

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.33%. Comparing base (301a894) to head (f438cef).
Report is 19 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1141   +/-   ##
=======================================
  Coverage   95.33%   95.33%           
=======================================
  Files          90       90           
  Lines       22700    22702    +2     
=======================================
+ Hits        21641    21644    +3     
+ Misses       1059     1058    -1     
Files with missing lines Coverage Δ
desc/objectives/_coils.py 99.14% <100.00%> (+<0.01%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Contributor

github-actions bot commented Jul 23, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     -1.33 +/- 7.00     | -7.30e-03 +/- 3.85e-02 |  5.43e-01 +/- 2.2e-02  |  5.50e-01 +/- 3.2e-02  |
 test_build_transform_fft_midres         |     +0.45 +/- 5.62     | +2.85e-03 +/- 3.52e-02 |  6.30e-01 +/- 1.9e-02  |  6.27e-01 +/- 3.0e-02  |
 test_build_transform_fft_highres        |     +0.71 +/- 3.26     | +7.17e-03 +/- 3.29e-02 |  1.02e+00 +/- 1.5e-02  |  1.01e+00 +/- 2.9e-02  |
 test_equilibrium_init_lowres            |     +2.27 +/- 3.82     | +8.95e-02 +/- 1.50e-01 |  4.03e+00 +/- 8.8e-02  |  3.94e+00 +/- 1.2e-01  |
 test_equilibrium_init_medres            |     +2.26 +/- 4.02     | +9.57e-02 +/- 1.70e-01 |  4.33e+00 +/- 1.4e-01  |  4.23e+00 +/- 9.2e-02  |
 test_equilibrium_init_highres           |     +0.96 +/- 2.55     | +5.35e-02 +/- 1.42e-01 |  5.63e+00 +/- 9.5e-02  |  5.58e+00 +/- 1.1e-01  |
 test_objective_compile_dshape_current   |     +0.16 +/- 2.27     | +6.33e-03 +/- 8.89e-02 |  3.93e+00 +/- 7.4e-02  |  3.92e+00 +/- 4.9e-02  |
 test_objective_compile_atf              |     +1.80 +/- 1.47     | +1.44e-01 +/- 1.18e-01 |  8.17e+00 +/- 8.8e-02  |  8.03e+00 +/- 7.8e-02  |
 test_objective_compute_dshape_current   |     -0.97 +/- 2.20     | -3.37e-05 +/- 7.66e-05 |  3.45e-03 +/- 5.4e-05  |  3.49e-03 +/- 5.5e-05  |
 test_objective_compute_atf              |     +0.59 +/- 5.83     | +6.18e-05 +/- 6.06e-04 |  1.05e-02 +/- 2.9e-04  |  1.04e-02 +/- 5.3e-04  |
 test_objective_jac_dshape_current       |     +3.46 +/- 7.94     | +1.36e-03 +/- 3.13e-03 |  4.07e-02 +/- 2.3e-03  |  3.93e-02 +/- 2.1e-03  |
 test_objective_jac_atf                  |     +2.12 +/- 3.37     | +4.02e-02 +/- 6.39e-02 |  1.94e+00 +/- 3.6e-02  |  1.90e+00 +/- 5.3e-02  |
 test_perturb_1                          |     -0.67 +/- 3.53     | -8.65e-02 +/- 4.54e-01 |  1.28e+01 +/- 3.6e-01  |  1.29e+01 +/- 2.8e-01  |
 test_perturb_2                          |     -0.49 +/- 2.72     | -8.75e-02 +/- 4.90e-01 |  1.79e+01 +/- 2.9e-01  |  1.80e+01 +/- 4.0e-01  |
 test_proximal_jac_atf                   |     -0.27 +/- 0.87     | -2.20e-02 +/- 7.14e-02 |  8.21e+00 +/- 6.2e-02  |  8.24e+00 +/- 3.5e-02  |
 test_proximal_freeb_compute             |     +0.01 +/- 0.95     | +2.27e-05 +/- 1.74e-03 |  1.83e-01 +/- 1.4e-03  |  1.83e-01 +/- 9.6e-04  |
 test_proximal_freeb_jac                 |     +1.32 +/- 0.94     | +9.91e-02 +/- 7.04e-02 |  7.63e+00 +/- 5.2e-02  |  7.53e+00 +/- 4.7e-02  |
 test_solve_fixed_iter                   |     +0.81 +/- 61.89    | +4.12e-02 +/- 3.13e+00 |  5.11e+00 +/- 2.1e+00  |  5.07e+00 +/- 2.3e+00  |

@f0uriest
Copy link
Member

could do something like:

for coil in coils:
    errorif(not isinstance(coil, _Coil),
            TypeError,
            f"Expected object of type Coil, got {type(coil)}",
)

that way it actually says what the offending member is.

@kianorr kianorr marked this pull request as ready for review August 16, 2024 23:03
@kianorr kianorr marked this pull request as draft August 19, 2024 20:57
@kianorr kianorr marked this pull request as ready for review August 20, 2024 18:35
Copy link
Collaborator

@dpanici dpanici left a comment

Choose a reason for hiding this comment

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

add a test

@dpanici dpanici self-requested a review August 21, 2024 20:25
@dpanici dpanici merged commit 04e6e56 into master Aug 28, 2024
18 checks passed
@dpanici dpanici deleted the ko/coil-error branch August 28, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow CoilXXX objectives also target curves, or add error if a curve is passed in
3 participants