search

luigibonati / mlcolvar

A unified framework for machine learning collective variables for enhanced sampling simulations
MIT License
89 stars 24 forks source link

Issues in reduced rank algorithm for TICA #54

Open luigibonati opened 1 year ago

luigibonati commented 1 year ago

There are two issues in the reduced_rank algorithm. 1) if the rank is equal to the number of features (or larger) it should fall back to least_squares, however, I think that as it is written now it just skips the calculation. Indeed, once it enters the first if statement it does not enter also in the following elif. 2) even when selecting rank < num features the algorithm throws an error. This is the result of executing the test_reduced_rank_tica function:

Traceback (most recent call last):
  File "/home/lbonati@iit.local/work/code/mlcvs/mlcolvar/tests/test_core_stats_tica.py", line 7, in <module>
    test_reduced_rank_tica()
  File "/home/lbonati@iit.local/work/code/mlcvs/mlcolvar/core/stats/tica.py", line 190, in test_reduced_rank_tica
    tica.compute([x_t,x_lag],[w_t,w_lag], save_params=True, algorithm='reduced_rank')
  File "/home/lbonati@iit.local/work/code/mlcvs/mlcolvar/core/stats/tica.py", line 89, in compute
    evals, evecs = reduced_rank_eig(C_0, C_lag, self.reg_C_0, rank = self.out_features)
  File "/home/lbonati@iit.local/work/code/mlcvs/mlcolvar/core/stats/utils.py", line 82, in reduced_rank_eig
    _, idxs = torch.topk(vectors.values, rank)
TypeError: topk(): argument 'input' (position 1) must be Tensor, not builtin_function_or_method

@Pietronvll can you check this? thanks!

luigibonati commented 1 year ago

if the rank is equal to the number of features (or larger) it should fall back to least_squares, however, I think that as it is written now it just skips the calculation. Indeed, once it enters the first if statement it does not enter also in the following elif.

I should have fixed the first point, check the related commit

luigibonati commented 1 year ago

@Pietronvll I merged the lightning branch into main since we plan to make a release later today. if you open a pull request target the main branch, thanks!

luigibonati commented 1 year ago

Since this has not been fixed I removed this feature from the main branch and put it into a separate reduced_rank branch. then we can open a pull request to put it back once it is working

I also moved the tests from the bottom of the file to the tests folder to use pytest fixtures to test different algorithms with the same function