You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Now that the local trapping paper is done, I want to documenting some potential issues from an email conversation between @akaptano, @MPeng5 for me (or someone else) to return to later. (This list doesn't include the alternate algorithm of Goyal, which would obviate some items)
Gradient of m in the update:
In cleaning the names for variables, I tried to create a grad_m variable here. However, I think that's the negative gradient, meaning we're doing gradient ascent on trap_center($m$). This is in the fix_grad_mbranch. Need to review equation 35 in Alan's original trapping paper.
edit: this may be due to a mistake in naming whether trap_center is actually m or trap_offset$=-m$? The current direction makes things work, and reversing it causes the problem to diverge.
Symmetrization and enstrophy calculation:
We currently create As using PM, not PQ. PM creates the symmetrization in the first two axes, because As is the symmetric part, and this worked prior to adding enstrophy. But with enstrophy, we should symmetrize after convert to the enstrophy basis. See the fix-symm-order branch.
Updating proxy enstrophy quadratic terms, $A^s$
The original paper describes $m$ and $A^s$ in turn, using the eigenvectors from $P \Xi$, but the algorithm in the paper uses eigenvectors merely from projecting onto the negative semidefinite cone. Meanwhile, the expressions earlier in the paper minimize them jointly. The current code uses eigenvectors from $P \Xi$, but only takes a single gradient step rather than minimizing either completely. How this happens doesn't seem a huge deal, but a potential alternative is in the project_A branch.
Note that I've tried to disambiguate between the relaxed $A^s$ formed from a set of coefficients and a trap center and the projection onto negative definite cone (which I refer to as "proxy").
Apply constraints to $\tilde H^0$ or $H^0$
These may be equivalent. We've modified the local trapping to regularize $\tilde H^0$, but global trapping still applies constraints to $H^0$. Check the h0tilde-constraint branch.
Doesn't appear to make much of a difference, but may not be correctly implemented
Other simplifications
Several other things would be nice to fix in trapping:
Making TrappingSR3 accept a PolynomialLibrary input, which can replace all of the feature-library-specific initialization parameters like _include_bias, _n_tgts, etc. It would also simplify all the functions that require a polyterms, made from PolynomialLibrary.powers_
Remove target-format constraints from pysindy. This was only a convenience for people that had written notebooks with a custom library. We don't need to maintain both C and F style ordering for flattened feature libraries.
Move constraint specification to fit() rather than __init__. Constraints can only really be used once the feature order is known, and they're only used by researchers deeply familiar with feature library API. This will remove complex logic from initialization, but needs to be done first in ConstrainedSR3.
Way forwards
Adding benchmarks, once the benchmarking branch is merged, would allow us to test out changes across a variety of problems in CI, rather than via manually running notebooks. That, and the other simplifications, should be the next steps
The text was updated successfully, but these errors were encountered:
Now that the local trapping paper is done, I want to documenting some potential issues from an email conversation between @akaptano, @MPeng5 for me (or someone else) to return to later. (This list doesn't include the alternate algorithm of Goyal, which would obviate some items)
Gradient of
m
in the update:In cleaning the names for variables, I tried to create a$m$ ). This is in the
grad_m
variable here. However, I think that's the negative gradient, meaning we're doing gradient ascent ontrap_center
(fix_grad_m
branch. Need to review equation 35 in Alan's original trapping paper.edit: this may be due to a mistake in naming whether$=-m$ ? The current direction makes things work, and reversing it causes the problem to diverge.
trap_center
is actuallym
or trap_offsetSymmetrization and enstrophy calculation:
We currently create As using PM, not PQ. PM creates the symmetrization in the first two axes, because As is the symmetric part, and this worked prior to adding enstrophy. But with enstrophy, we should symmetrize after convert to the enstrophy basis. See the fix-symm-order branch.
Updating proxy enstrophy quadratic terms,$A^s$
The original paper describes$m$ and $A^s$ in turn, using the eigenvectors from $P \Xi$ , but the algorithm in the paper uses eigenvectors merely from projecting onto the negative semidefinite cone. Meanwhile, the expressions earlier in the paper minimize them jointly. The current code uses eigenvectors from $P \Xi$ , but only takes a single gradient step rather than minimizing either completely. How this happens doesn't seem a huge deal, but a potential alternative is in the project_A branch.
Note that I've tried to disambiguate between the relaxed$A^s$ formed from a set of coefficients and a trap center and the projection onto negative definite cone (which I refer to as "proxy").
Apply constraints to$\tilde H^0$ or $H^0$
These may be equivalent. We've modified the local trapping to regularize$\tilde H^0$ , but global trapping still applies constraints to $H^0$ . Check the h0tilde-constraint branch.
Doesn't appear to make much of a difference, but may not be correctly implemented
Other simplifications
Several other things would be nice to fix in trapping:
_include_bias
,_n_tgts
, etc. It would also simplify all the functions that require a polyterms, made from PolynomialLibrary.powers_fit()
rather than__init__
. Constraints can only really be used once the feature order is known, and they're only used by researchers deeply familiar with feature library API. This will remove complex logic from initialization, but needs to be done first inConstrainedSR3
.Way forwards
Adding benchmarks, once the benchmarking branch is merged, would allow us to test out changes across a variety of problems in CI, rather than via manually running notebooks. That, and the other simplifications, should be the next steps
The text was updated successfully, but these errors were encountered: