-
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
add basic coil objectives #853
Conversation
| benchmark_name | dt(%) | dt(s) | t_new(s) | t_old(s) |
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
test_build_transform_fft_lowres | +8.77 +/- 4.22 | +1.58e-01 +/- 7.58e-02 | 1.95e+00 +/- 6.4e-02 | 1.80e+00 +/- 4.0e-02 |
test_build_transform_fft_midres | +2.93 +/- 4.46 | +6.01e-02 +/- 9.16e-02 | 2.11e+00 +/- 5.6e-02 | 2.05e+00 +/- 7.2e-02 |
test_build_transform_fft_highres | +3.48 +/- 3.96 | +8.39e-02 +/- 9.55e-02 | 2.50e+00 +/- 7.7e-02 | 2.41e+00 +/- 5.6e-02 |
test_equilibrium_init_lowres | +2.09 +/- 5.13 | +2.15e-01 +/- 5.27e-01 | 1.05e+01 +/- 3.9e-01 | 1.03e+01 +/- 3.5e-01 |
test_equilibrium_init_medres | +2.73 +/- 3.06 | +2.98e-01 +/- 3.35e-01 | 1.12e+01 +/- 2.5e-01 | 1.09e+01 +/- 2.3e-01 |
test_equilibrium_init_highres | +3.98 +/- 3.35 | +4.96e-01 +/- 4.17e-01 | 1.29e+01 +/- 3.4e-01 | 1.25e+01 +/- 2.5e-01 |
test_objective_compile_dshape_current | -0.94 +/- 1.71 | -3.82e-02 +/- 6.92e-02 | 4.01e+00 +/- 5.4e-02 | 4.05e+00 +/- 4.3e-02 |
test_objective_compile_atf | +0.20 +/- 5.47 | +1.52e-02 +/- 4.17e-01 | 7.64e+00 +/- 3.4e-01 | 7.63e+00 +/- 2.5e-01 |
test_objective_compute_dshape_current | +0.42 +/- 7.64 | +1.90e-05 +/- 3.48e-04 | 4.58e-03 +/- 3.3e-04 | 4.56e-03 +/- 1.1e-04 |
test_objective_compute_atf | +2.59 +/- 3.98 | +4.71e-04 +/- 7.24e-04 | 1.87e-02 +/- 6.6e-04 | 1.82e-02 +/- 2.9e-04 |
test_objective_jac_dshape_current | -2.07 +/- 7.25 | -8.75e-04 +/- 3.06e-03 | 4.13e-02 +/- 2.4e-03 | 4.22e-02 +/- 1.9e-03 |
test_objective_jac_atf | +1.54 +/- 3.16 | +2.84e-02 +/- 5.83e-02 | 1.87e+00 +/- 4.6e-02 | 1.84e+00 +/- 3.6e-02 |
test_perturb_1 | +0.97 +/- 3.79 | +1.51e-01 +/- 5.86e-01 | 1.56e+01 +/- 4.9e-01 | 1.55e+01 +/- 3.2e-01 |
test_perturb_2 | +2.06 +/- 1.67 | +4.23e-01 +/- 3.43e-01 | 2.10e+01 +/- 3.1e-01 | 2.05e+01 +/- 1.5e-01 |
test_proximal_jac_atf | +0.52 +/- 1.68 | +3.74e-02 +/- 1.21e-01 | 7.21e+00 +/- 1.0e-01 | 7.17e+00 +/- 6.4e-02 |
test_proximal_freeb_compute | -0.08 +/- 1.09 | -9.54e-05 +/- 1.30e-03 | 1.20e-01 +/- 7.8e-04 | 1.20e-01 +/- 1.0e-03 |
test_proximal_freeb_jac | -2.01 +/- 2.36 | -1.49e-01 +/- 1.74e-01 | 7.23e+00 +/- 7.6e-02 | 7.38e+00 +/- 1.6e-01 | |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #853 +/- ##
==========================================
+ Coverage 94.62% 94.64% +0.01%
==========================================
Files 86 87 +1
Lines 21606 21740 +134
==========================================
+ Hits 20444 20575 +131
- Misses 1162 1165 +3
|
tests that coil length of radius 1 is 2pi
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.
Merge #840 and see if it works
changed grid to N=32. Everything is passing
Might want to merge again, idt any tests should fail from the compute function side |
some problems with merge conflicts so we'll see if it works
tests that coil length of radius 1 is 2pi
changed grid to N=32. Everything is passing
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 get another review from @dpanici before merging
I would like to look at it too, if you don't mind waiting a day |
Of course, go for it but I'd like to merge it sooner rather than later |
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.
change the docstring to address Rory's previous comment on loss functions
my bad, forgot to change it for all of them. thanks |
Instead of "Affects" can you say "operates over all coils, not each individial coil"? to make it more clear that it will make the error a scalar value, not a single value per coil |
|
||
opt = Optimizer("fmintr") | ||
coil = FourierRZCoil() | ||
coil.change_resolution(N=1) |
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.
If we are targeting a constant curvature value everywhere and only allowing a single Fourier mode, can we also check that the Fourier coefficient is the value we expect after the optimization? And/or allow more Fourier modes in the optimization and check that those higher order modes are close to zero?
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.
FourierRZCurve
is non-planar, so there are multiple shapes that have constant curvature, so I don't think there is an "expected" value for any of the coefficients.
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.
Adds coil length objective
Resolves #895
Resolves #897
Resolves #896
Resolves #863