Clean the Trapping variable names, #553
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes it easier to reason about the problem at different steps
Note: Using einsum in place of tensordot because in tensordot:
The shape of the result consists of the non-contracted axes of the first
tensor, followed by the non-contracted axes of the second.
Whenever we multiply a projection matrix A like P^{1/2} A P^{-1/2}, using
tensordot means that the last axis of P^{-1/2} comes last, requiring an
additional transpose step. Using einsum skips that step. See:
```python
import numpy as np
arr1 = np.random.rand(1, 1, 1, 2)
arr2 = np.random.rand(1, 1)
opt1 = np.tensordot(arr2, arr1, axes=([1], [0]))
opt1 = np.tensordot(opt1, arr2, axes=([1], [0]))
opt1 = np.transpose(opt1, axes=[0, 3, 1, 2])
opt2 = np.einsum("ya,abcd,bz->yzcd", arr2, arr1, arr2)
np.testing.assert_allclose(opt1, opt2) # passes
```
…should be multiplied by sqrt(mod_matrix) not mod_matrix, and added in the extra sqrt(P) factors in H0tilde term that gets minimized. All the notebooks working well for the local algorithm, including with the enstrophy, but global with enstrophy is still not working well. Possible reason is that the constraint is still on H0 and not on H0tilde.
…ation of Q_ep in the notebook
This also involves creating a simple EnstrophyMat class. A lot of operations carry around the precomputed (inv)root, and we don't need a whole trapping optimizer to check those operations.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #553 +/- ##
==========================================
+ Coverage 94.64% 95.30% +0.65%
==========================================
Files 37 37
Lines 4018 4046 +28
==========================================
+ Hits 3803 3856 +53
+ Misses 215 190 -25 ☔ View full report in Codecov by Sentry. |
Not necessarily the best test, but coverage should at least find any shape errors that arise. Also, remove tests for different regularizers from trapping, now that that regularization is fully abstracted to superclass, with exception of reg == 0 vs reg != 0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Most significant changes are:
_equality_constrained_linlsq())_permutation_asymmetry()), conversion of Q to enstrophy basis (_convert_quad_terms_to_ens_basis()), and creating As (_create_A_symm())EnstrophyMatclass (simplifies parameter signatures without relying on methods)Most of these changes improve the testability of the code, although tests are not written for all of them (yet?). Furthermore,
_solve_m_relax_and_split()needs work, but may need to be changed