ag/surface_dofs_fix

[andrewgiuliani]

In this PR, I propose a fix for an issue that @ejpaul pointed out. Python complains if you try to compute a derivative with BoozerSurfaces when a coil degree of freedom is fixed. We didn't have any unit tests for this, but I have added one now.

I have proposed a not very elegant modification of the __call__ function of the Derivative class.

The issue is that simsopt currently filters out the partial derivatives of the fixed dofs. When the user requests it to return a Derivative object, rather than a numpy array of the derivative, this can cause issues.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.64%. Comparing base (11f176b) to head (b1350c2).
Report is 5 commits behind head on master.

❗ Current head b1350c2 differs from pull request most recent head 9039141

Please upload reports for the commit 9039141 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
- Coverage   91.99%   91.64%   -0.36%     
==========================================
  Files          75       75              
  Lines       13464    13449      -15     
==========================================
- Hits        12386    12325      -61     
- Misses       1078     1124      +46     

Flag	Coverage Δ
unittests	91.64% <100.00%> (-0.36%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[landreman]

Can a test be added in tests/core/test_derivative.py to cover this issue using the little optimizable functions there without relying on BoozerSurface?

[landreman]

Here's another thought. Instead of having call() return derivatives with respect to only free dofs if as_derivative==False and return derivatives with respect to all dofs (fixed and free) if as_derivative==True, an alternative option could be to add a new keyword argument to call() that controls whether the returned data includes only fixed dofs or all dofs. Suppose this new keyword argument was named free_only and was True by default. Then users would have the option of both call(as_derivative=True, free_only=True) and call(as_derivative=True, free_only=False). Would you foresee both of these cases as being potentially useful? I'm not saying we have to go this route, just wanted to put this option on the table.

It looks to me like as_derivative=True is used only in one place in all of simsopt, in surface_objectives.py:boozer_surface_dexactresidual_dcoils_dcurrents_vjp(). So not much will be affected by the choice of how as_derivative=True behaves.

[andrewgiuliani]

Can a test be added in tests/core/test_derivative.py to cover this issue using the little optimizable functions there without relying on BoozerSurface?

This is checked now in test_as_derivative2. The issue was that two Derivative dictionaries are summed together on line 1088:

simsopt/src/simsopt/geo/surfaceobjectives.py

Lines 1086 to 1088 in eb6b27f

	lm_times_dlabel_dcoils = lm_label*booz_surf.label.dJ(partials=True)(biotsavart, as_derivative=True)
	return lm_times_dres_dcoils+lm_times_dlabel_dcoils

The first Derivative dictionary had values with the full set derivatives wrt both fixed and free DOFs. The second dictionary had values only wrt to the free DOFs obtained from the as_derivative=True keyword argument, hence the conflict. test_as_derivative2 simulates this by summing one Derivative obtained in the standard way, and another obtained using the as_derivative=True option.

Here's another thought. Instead of having call() return derivatives with respect to only free dofs if as_derivative==False and return derivatives with respect to all dofs (fixed and free) if as_derivative==True, an alternative option could be to add a new keyword argument to call() that controls whether the returned data includes only fixed dofs or all dofs. Suppose this new keyword argument was named free_only and was True by default. Then users would have the option of both call(as_derivative=True, free_only=True) and call(as_derivative=True, free_only=False). Would you foresee both of these cases as being potentially useful? I'm not saying we have to go this route, just wanted to put this option on the table.

I decided against adding another keyword argument for free_only=False because this will result in two kinds of Derivative dictionaries, ones with derivatives wrt to all dofs and others with derivatives only wrt to free dofs. These two kinds of Derivatives cannot be summed together without causing a numpy error (due to summing two numpy arrays of different length). This is highlighted by the unit test test_as_derivative2. I think it's more intuitive for the user to always have dictionaries with keys having the derivative wrt to the full set of DOFs, this way Derivatives can always be summed together without issues.

Therefore, when as_derivative=True, I have opted to always return a Derivative dictionary that has data with respect to the full set of DOFs.