support not pre-compute cov #21
Conversation
mtrf/matrices.py
Outdated
| @@ -1,6 +1,19 @@ | |||
| import numpy as np | |||
|
|
|||
|
|
|||
| def _reduced_covariance_matrices(xs, ys, lags, zeropad=True, bias=True): | |||
There was a problem hiding this comment.
Do we need an extra function for this? We could also add a boolean argument average to the covariance_matrices function
There was a problem hiding this comment.
I feel adding too much functionality into the covariance_matrices will make it too complicated (additional if..else.. branch). And because we need to accumulatively sum the cov_mat for each trial (to save memory), we still need to add several lines of code into the covariance_matrices, given that currently we use a list to store the cov_mat for each trial.
mtrf/model.py
Outdated
| If False, delete them. | ||
| method: str | ||
| Regularization method. Can be 'ridge' (default), 'tikhonov' or 'banded'. | ||
| See documentation for a detailed explanation. | ||
| pre_cal_cov: bool |
There was a problem hiding this comment.
Maybe just call it 'precompute'? The details will be in the docstrings anyway and I think too many abbreviations make the code opaque
There was a problem hiding this comment.
Yeah...., what about calling it savemem? (to directly indicate that this option is for saving memory)
mtrf/model.py
Outdated
| ): | ||
| self.weights = None | ||
| self.bias = bias | ||
| self.times = None | ||
| self.fs = None | ||
| self.regularization = None | ||
| if isinstance(pre_cal_cov, bool): |
There was a problem hiding this comment.
I think we should either check the types of all or arguments or none
mtrf/model.py
Outdated
| @@ -363,12 +389,17 @@ def test( | |||
| "Hyperparameter optimization", | |||
| verbose=verbose, | |||
| ): | |||
| cov_xx_this = None | |||
There was a problem hiding this comment.
This feels not very informative, maybe cov_xx_train?
|
I like the idea but I think its a lot of code for very little functionality - maybe we can simplify things by giving the existing |
support not pre-compute covmat, by delegating the functionality to the _cross_validate function. Also add corresponding test case in the tests