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

Yet another single-stage refactor #319

Merged
merged 4 commits into from
Jun 6, 2023

Conversation

landreman
Copy link
Contributor

This is yet another possible refactoring of the single-stage example in #301 suggested by @mbkumar, as an alternative to #317. In this approach, MPIFiniteDifference broadcasts full_x instead of just the free x, so there is no extra data that needs to be broadcast. The coil dofs are fixed during the finite-differencing, then un-fixed to update the coil dofs and compute their gradient analytically, then fixed again for the next finite-differencing, etc. Even though the coil dofs are fixed during the finite differencing, they are still part of full_x, so they get broadcast.

To get this working, a few functions were added to Optimizable: a setter for full_x (copied from @mbkumar 's commit to #317), and full_fix() and full_unfix() to set the free/fixed property for the full dofs on which an object depends. Corresponding tests have been added. A few changes from #301 to finite_difference.py and optimizable.py have been reverted, as in #317.

For single_stage_optimization_finite_beta.py, this branch gives identical values for all quantities at the end of the optimization and uses fewer calls to VirtualCasing.from_vmec() compared to #301.

I don't have a super strong preference between this approach and #317, but perhaps slightly prefer this one, since it makes sense that full_x should be synched by MPIFiniteDifference anyway. What do you think?

@landreman landreman requested review from mbkumar and rogeriojorge June 4, 2023 11:11
@codecov
Copy link

codecov bot commented Jun 4, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.40 ⚠️

Comparison is base (dbf0232) 90.78% compared to head (7dfa31d) 90.38%.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           rj/single_stage_PR     #319      +/-   ##
======================================================
- Coverage               90.78%   90.38%   -0.40%     
======================================================
  Files                      63       63              
  Lines                   10719    10732      +13     
======================================================
- Hits                     9731     9700      -31     
- Misses                    988     1032      +44     
Flag Coverage Δ
unittests 90.38% <100.00%> (-0.40%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/simsopt/_core/finite_difference.py 93.22% <100.00%> (-0.88%) ⬇️
src/simsopt/_core/optimizable.py 90.35% <100.00%> (+0.18%) ⬆️

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rogeriojorge rogeriojorge merged commit 11e6e93 into rj/single_stage_PR Jun 6, 2023
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.

3 participants