simsopt icon indicating copy to clipboard operation
simsopt copied to clipboard

Rewriten finite difference logging

Open daringli opened this issue 3 years ago • 1 comments

Rewrites part of MPIFiniteDifference and FiniteDifference to:

  • Support Jacobian calculations for both scipy.optimize.minimize and scipy.optimize.least_squares. The former needs df/dx_i and the latter dR_i/dx_j, where R_i are the components of the residual vector.
  • Separates the logging of the calculated Jacobian from the calculation. The logging is now done by a decorator to (MPI)FiniteDifference.jac. This lets us log details about the solver that FiniteDifference may not know about.
  • Supports several different options for logging the Jacobian: the current format (default), df/dx_i and dR_i/dx_j (only for least_squares).
  • Makes it possible to use FD derivatives with simsopt.solve.serial.serial_solve. Updates this function to work with the latest version of simsopt.

The current format of the Jacobian log only logs the points at which the objective was evaluated to calculate the Jacobian, and not the actual function value at these points. For more expensive objectives (such as those used for direct turbulence optimization), it becomes non-trivial to reevaluate the Jacobian, so the output is not as useful.

A concern I have is that this pull request currently introduces some "switch-case based programming", where the (MPI)FiniteDifference objects and the finite_difference_jac_decorator uses if-clauses to adjust their outputs, partly based on the needs of the different solvers. A different approach would be to embrace that MPIFiniteDifference and FiniteDifference are only meant for use with scipy.optimize.least_squares, and create a derived class FiniteDifference_minimize that overrides the jac method to flatten the output for scipy.optimize.minimize. The logging could then again be done in the FiniteDifference objects themselves, since they would be specific to each solver and thus could print hard-coded solver info. Users using different solvers would then create derived FiniteDifference classes with different solver information. Input and ideas are appreciated.

daringli avatar Jan 11 '23 20:01 daringli

Codecov Report

Base: 91.52% // Head: 91.28% // Decreases project coverage by -0.23% :warning:

Coverage data is based on head (f52f8b8) compared to base (2553989). Patch coverage: 75.83% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #283      +/-   ##
==========================================
- Coverage   91.52%   91.28%   -0.24%     
==========================================
  Files          61       61              
  Lines        9249     9328      +79     
==========================================
+ Hits         8465     8515      +50     
- Misses        784      813      +29     
Flag Coverage Δ
unittests 91.28% <75.83%> (-0.24%) :arrow_down:

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

Impacted Files Coverage Δ
src/simsopt/field/magneticfieldclasses.py 96.01% <ø> (ø)
src/simsopt/solve/serial.py 59.32% <23.07%> (-0.86%) :arrow_down:
src/simsopt/_core/finite_difference.py 86.62% <80.30%> (-7.24%) :arrow_down:
src/simsopt/solve/mpi.py 93.16% <100.00%> (+0.17%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Jan 12 '23 00:01 codecov[bot]