Skip to content

Add eq_fixed option to BoundaryError#2264

Open
IssraAli wants to merge 11 commits into
masterfrom
ia/BoundaryError-eq_fixed
Open

Add eq_fixed option to BoundaryError#2264
IssraAli wants to merge 11 commits into
masterfrom
ia/BoundaryError-eq_fixed

Conversation

@IssraAli

Copy link
Copy Markdown
Collaborator

An option to remove the equilibrium as an optimizable DOF when computing BoundaryError is included.

@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Memory benchmark result

|               Test Name                |      %Δ      |    Master (MB)     |      PR (MB)       |    Δ (MB)    |    Time PR (s)     |  Time Master (s)   |
| -------------------------------------- | ------------ | ------------------ | ------------------ | ------------ | ------------------ | ------------------ |
  test_objective_jac_w7x                 |    2.59 %    |     3.846e+03      |     3.945e+03      |    99.66     |       32.55        |       29.23        |
  test_proximal_jac_w7x_with_eq_update   |   -0.23 %    |     6.521e+03      |     6.506e+03      |    -15.07    |       156.20       |       152.46       |
  test_proximal_freeb_jac                |   -0.05 %    |     1.340e+04      |     1.339e+04      |    -6.89     |       84.76        |       79.31        |
  test_proximal_freeb_jac_blocked        |   -0.51 %    |     7.729e+03      |     7.689e+03      |    -39.58    |       71.10        |       69.08        |
  test_proximal_freeb_jac_batched        |   -0.33 %    |     7.646e+03      |     7.621e+03      |    -25.32    |       71.30        |       67.98        |
  test_proximal_jac_ripple               |    0.79 %    |     3.536e+03      |     3.563e+03      |    27.81     |       56.16        |       53.35        |
  test_proximal_jac_ripple_bounce1d      |   -1.44 %    |     3.798e+03      |     3.744e+03      |    -54.83    |       68.32        |       69.74        |
  test_eq_solve                          |    0.19 %    |     1.950e+03      |     1.954e+03      |     3.75     |       58.00        |       58.48        |

For the memory plots, go to the summary of Memory Benchmarks workflow and download the artifact.

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.30%. Comparing base (7b6ed16) to head (87c4602).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
desc/objectives/_free_boundary.py 85.71% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2264      +/-   ##
==========================================
- Coverage   94.30%   94.30%   -0.01%     
==========================================
  Files         101      101              
  Lines       28866    28901      +35     
==========================================
+ Hits        27223    27255      +32     
- Misses       1643     1646       +3     
Files with missing lines Coverage Δ
desc/objectives/_free_boundary.py 94.93% <85.71%> (-1.54%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@YigitElma YigitElma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can also add a test like test_toroidal_flux to check all paths run fine.

Comment thread desc/objectives/_free_boundary.py Outdated
@YigitElma

Copy link
Copy Markdown
Collaborator

I think the reason tests are failing is that constants["field"] is a SumMagneticField and it expects a list for the params. So, the parameter assignment needs a bit more care. For this objective, field can be something like [MixedCoilSet(), ToroidalMagneticField()].

@IssraAli IssraAli requested a review from YigitElma July 14, 2026 15:58

@YigitElma YigitElma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

About the formatting and linting check, if you run the following, they will happen automatically everytime you commit something.

pip install pre-commit
pre-commit install

These and some other useful instructions can be found on the Contributing Guide.

Comment thread desc/objectives/_free_boundary.py Outdated
Comment thread desc/objectives/_free_boundary.py Outdated
Comment thread tests/test_objective_funs.py Outdated
Comment thread tests/test_objective_funs.py Outdated
Comment thread CHANGELOG.md Outdated

@YigitElma YigitElma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Note to other devs: The failing test is related to jax-finufft version 1.2.0 and not caused by this PR. We should solve that problem separetaly or wait CI to run that test on a different python version (which doesn't install jax-finufft 1.2.0)

Comment thread CHANGELOG.md

New Features

- Added option for ``eq_fixed`` added to ``BoundaryError`` to remove equilibrium as a degree of freedom from boundary error objective.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Added option for ``eq_fixed`` added to ``BoundaryError`` to remove equilibrium as a degree of freedom from boundary error objective.
- Adds ``eq_fixed`` argument to ``BoundaryError`` to remove the equilibrium from the optimization. This can be used instead of adding a ``FixParameter(eq)`` constraint.

Or you can just fix the typo

@YigitElma YigitElma requested review from ddudt, dpanici and f0uriest July 15, 2026 22:28
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.

2 participants