search

markovmodel / variational

Basis sets, estimators and solvers for the variational approach of conformation dynamics. NOTE: the code has been merged with PyEMMA and is maintained there.
10 stars 5 forks source link

Fixed error in _M2_const if one of the two masks is None. #26

Closed fnueske closed 8 years ago

fnueske commented 8 years ago

Hi, I found that variational/estimators/moments.py/_M2_const can crash if either of mask_X or mask_Y is None. This can happen as the use of _M2_const is only ruled out in _M2 if both are None.

I have included this simple check which just replaces the mask which is None by the list of all columns. @franknoe , is it okay like this or would you like to solve this differently?

franknoe commented 8 years ago

This solution is correct, although it might be inefficient to use this function in the case that one of them is None. The _xxx functions are only meant to be used from inside the module, so I guess this case doesn't occur if you use public functions (at least it isn't intended to happen).

However, the masks used here are boolean masks, so just for consistency reason I would suggest to create a boolean vector with all True (use np.ones(N, dtype=np.bool)) instead of using arange.

fnueske commented 8 years ago

It just did happen to me when using one of the API functions. I guess it is not likely, but could occur if something rare happens at the end or beginning of a short trajectory. I have replaced np.arange by a Boolean array now.

franknoe commented 8 years ago

OK. I can merge this, but I'd like to understand when this happens, because it isn't intended - so there might be another issue.

fnueske commented 8 years ago

You're right, it seems unlikely. I'll try to provide an example later.