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

Add mirror ratio objective #1366

Merged
merged 12 commits into from
Dec 13, 2024
Merged

Add mirror ratio objective #1366

merged 12 commits into from
Dec 13, 2024

Conversation

f0uriest
Copy link
Member

@f0uriest f0uriest commented Nov 14, 2024

Resolves #848

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

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

Project coverage is 95.62%. Comparing base (afeff2c) to head (df43f17).
Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
desc/objectives/_geometry.py 94.59% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1366      +/-   ##
==========================================
- Coverage   95.62%   95.62%   -0.01%     
==========================================
  Files          98       98              
  Lines       25374    25411      +37     
==========================================
+ Hits        24264    24299      +35     
- Misses       1110     1112       +2     
Files with missing lines Coverage Δ
desc/compute/_field.py 100.00% <ø> (ø)
desc/objectives/__init__.py 100.00% <ø> (ø)
desc/objectives/_geometry.py 96.79% <94.59%> (-0.23%) ⬇️

... and 3 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented Nov 14, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     -1.32 +/- 3.58     | -7.80e-03 +/- 2.12e-02 |  5.84e-01 +/- 1.6e-02  |  5.91e-01 +/- 1.4e-02  |
 test_equilibrium_init_medres            |     +0.31 +/- 2.19     | +1.46e-02 +/- 1.03e-01 |  4.71e+00 +/- 8.4e-02  |  4.70e+00 +/- 6.0e-02  |
 test_equilibrium_init_highres           |     -0.32 +/- 2.91     | -1.94e-02 +/- 1.74e-01 |  5.97e+00 +/- 1.4e-01  |  5.99e+00 +/- 1.0e-01  |
 test_objective_compile_dshape_current   |     +0.03 +/- 1.99     | +1.29e-03 +/- 8.43e-02 |  4.23e+00 +/- 4.4e-02  |  4.23e+00 +/- 7.2e-02  |
 test_objective_compute_dshape_current   |     -5.20 +/- 8.60     | -3.19e-04 +/- 5.27e-04 |  5.81e-03 +/- 2.3e-04  |  6.13e-03 +/- 4.7e-04  |
 test_objective_jac_dshape_current       |     +0.23 +/- 6.32     | +1.06e-04 +/- 2.89e-03 |  4.58e-02 +/- 2.0e-03  |  4.57e-02 +/- 2.1e-03  |
 test_perturb_2                          |     -0.21 +/- 1.82     | -4.50e-02 +/- 3.98e-01 |  2.18e+01 +/- 3.0e-01  |  2.18e+01 +/- 2.6e-01  |
 test_proximal_freeb_jac                 |     +0.02 +/- 1.72     | +1.82e-03 +/- 1.37e-01 |  7.94e+00 +/- 9.2e-02  |  7.94e+00 +/- 1.0e-01  |
 test_solve_fixed_iter                   |     -0.67 +/- 3.53     | -2.41e-01 +/- 1.27e+00 |  3.58e+01 +/- 8.7e-01  |  3.60e+01 +/- 9.3e-01  |
 test_LinearConstraintProjection_build   |     +0.24 +/- 2.91     | +2.70e-02 +/- 3.31e-01 |  1.14e+01 +/- 1.9e-01  |  1.14e+01 +/- 2.7e-01  |
 test_build_transform_fft_midres         |     -0.44 +/- 4.15     | -2.66e-03 +/- 2.51e-02 |  6.01e-01 +/- 1.6e-02  |  6.04e-01 +/- 1.9e-02  |
 test_build_transform_fft_highres        |     -0.29 +/- 2.92     | -2.82e-03 +/- 2.82e-02 |  9.63e-01 +/- 1.3e-02  |  9.65e-01 +/- 2.5e-02  |
 test_equilibrium_init_lowres            |     -3.04 +/- 9.03     | -1.21e-01 +/- 3.59e-01 |  3.85e+00 +/- 1.1e-01  |  3.97e+00 +/- 3.4e-01  |
 test_objective_compile_atf              |     -3.44 +/- 4.74     | -2.86e-01 +/- 3.94e-01 |  8.03e+00 +/- 2.8e-01  |  8.31e+00 +/- 2.8e-01  |
 test_objective_compute_atf              |     -3.30 +/- 4.00     | -5.39e-04 +/- 6.54e-04 |  1.58e-02 +/- 5.8e-04  |  1.63e-02 +/- 3.1e-04  |
 test_objective_jac_atf                  |     -1.09 +/- 1.26     | -2.14e-02 +/- 2.48e-02 |  1.95e+00 +/- 1.5e-02  |  1.97e+00 +/- 2.0e-02  |
 test_perturb_1                          |     -1.72 +/- 2.17     | -2.54e-01 +/- 3.20e-01 |  1.45e+01 +/- 2.4e-01  |  1.48e+01 +/- 2.1e-01  |
 test_proximal_jac_atf                   |     +0.04 +/- 1.12     | +3.15e-03 +/- 9.24e-02 |  8.23e+00 +/- 7.4e-02  |  8.22e+00 +/- 5.6e-02  |
 test_proximal_freeb_compute             |     +1.09 +/- 1.19     | +2.15e-03 +/- 2.35e-03 |  2.00e-01 +/- 1.4e-03  |  1.97e-01 +/- 1.9e-03  |
 test_solve_fixed_iter_compiled          |     +0.79 +/- 1.69     | +1.71e-01 +/- 3.65e-01 |  2.18e+01 +/- 1.5e-01  |  2.16e+01 +/- 3.3e-01  |

@f0uriest f0uriest marked this pull request as draft November 14, 2024 03:04
@f0uriest f0uriest marked this pull request as ready for review December 12, 2024 20:41
@f0uriest f0uriest requested review from a team, rahulgaur104, ddudt, dpanici, kianorr, sinaatalay, unalmis and YigitElma and removed request for a team December 12, 2024 20:41
ddudt
ddudt previously approved these changes Dec 12, 2024
Copy link
Collaborator

@ddudt ddudt left a comment

Choose a reason for hiding this comment

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

Approving, but would like to see if you can change the test to work with a lower tolerance

Comment on lines 1438 to 1439
# not perfect agreement bc eq is low res, so B isnt exactly B0/R
np.testing.assert_allclose(f, mirror_ratio, rtol=0.13)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be better to use the Solovev or D-shape examples. They aren't perfectly circular but are higher-resolution equilibria, so might give a more accurate solution anyways

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess increasing the resolution slightly can give better results without taking too much time.

Copy link
Collaborator

@YigitElma YigitElma left a comment

Choose a reason for hiding this comment

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

I agree with @ddudt, the tolerance is a bit high, but not too important if you don't change it. I made some suggestions that should work.

tests/test_objective_funs.py Outdated Show resolved Hide resolved
tests/test_objective_funs.py Outdated Show resolved Hide resolved
Comment on lines 1438 to 1439
# not perfect agreement bc eq is low res, so B isnt exactly B0/R
np.testing.assert_allclose(f, mirror_ratio, rtol=0.13)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess increasing the resolution slightly can give better results without taking too much time.

Copy link
Collaborator

@YigitElma YigitElma left a comment

Choose a reason for hiding this comment

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

Update changelog

Co-authored-by: Yigit Gunsur Elmacioglu <[email protected]>
@f0uriest f0uriest requested review from YigitElma and ddudt December 13, 2024 02:01
YigitElma
YigitElma previously approved these changes Dec 13, 2024
dpanici
dpanici previously approved these changes Dec 13, 2024
YigitElma
YigitElma previously approved these changes Dec 13, 2024
@f0uriest f0uriest dismissed stale reviews from YigitElma and dpanici via df43f17 December 13, 2024 19:41
@f0uriest f0uriest merged commit 7f9d4ee into master Dec 13, 2024
24 of 25 checks passed
@f0uriest f0uriest deleted the rc/mirror_ratio branch December 13, 2024 21:33
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.

Mirror Ratio objective
4 participants