t(emporal)PARAFAC2's temporal smoothness penalty

[cchatzis]

Implementation of temporal smoothness penalty, as presented in:

C. Chatzis, M. Pfeffer, P. Lind and E. Acar, "A Time-Aware Tensor Decomposition for Tracking Evolving Patterns," 2023 IEEE 33rd International Workshop on Machine Learning for Signal Processing (MLSP), Rome, Italy, 2023, pp. 1-6, doi: 10.1109/MLSP55844.2023.10285943.

This PR essentially adds the custom penalty class, (TemporalSmoothnessPenalty) to penalties.py.

[MarieRoald]

Hi @cchatzis,

Thank you for the PR! This looks like a great contribution -- a perfect fit for MatCoupLy!

I made a couple of small suggestions, including one that avoids a nested for loop for the A-assembly, which could speed it up for large I.

It would also be great with a test for this penalty. To add this, you need to add a test class that inherits from matcouply.testing.BaseTestFactorMatricesPenalty (it should already be imported in tests/test_penalties.py) and add three methods: test_penalty, get_invariant_matrices and get_non_invariant_matrices:

• test_penalty should test that the penalty is computed correctly, either based on a hard-coded example where you know what the penalty should be, or by computing it completely manually in the test
• get_invariant_matrices(rng, shapes) should generate a random list of matrices that won't be changed by the proximal operator (I guess it should be a stack of the same matrix repeated len(shapes) times?).
• get_non_invariant_matrices(rng, shapes) should generate a random list of matrices that will be changed by the proximal operator (I guess it could just be a list of random matrices).

It might be useful to set the class attribute min_rows = max_rows = 10 or something to ensure that all shapes in the shapes list are the same. The tests for the GeneralizedL2Penalty might also be a useful place to start: https://github.com/MarieRoald/matcouply/blob/main/tests/test_penalties.py#L247

Don't be afraid to ask if you have any other questions or if anything I said wasn't clear :)

[cchatzis]

Thank you very much for the thorough suggestions and help @MarieRoald!

I think I have addressed all of the raised issues and I added a small test test_A_assembly in tests/test_penalties.py that checks that both the efficient approach you proposed and "hard-coded" approach I had initially have the same result.

Kindly let me know what you think!

[MarieRoald]

Looks really good, just a couple of minor comments. The PyTorch are failing, but addressing the comments will hopefully make them pass :)

[cchatzis]

Hi, are there any updates on this? Thanks in advance :)

[MarieRoald]

Looks good, but we need to know if the tests run. For some reason, the CI/CD pipeline doesn't want to trigger, so I made some changes to the .github/workflows/Tests.yml on main.

I tried to push the changes directly on top of your fork, but I wasn't able to. So I think that either, you need to merge main into your feature branch or make it so I can push to your branch.

To merge, I think it should work if you:

1. Make sure that my repo is added as a remote. Run git remote -v to list all remotes, there should be a remote with the name upstream (or something similar) that points to my repo. If there is no remote with the url https://github.com/MarieRoald/matcouply.git (or the SSH-equivalent), then you should run git remote add upstream https://github.com/MarieRoald/matcouply.git.
2. Check out main and pull latest changes: git checkout main followed by git pull upstream.
3. Finally, check out your branch and merge main: git checkout tPARAFAC2 followed by git merge main.