search

stefanonardo / pytorch-esn

An Echo State Network module for PyTorch.
MIT License
211 stars 43 forks source link

risky judgment on solving the output with 'inv' method #10

Closed XinzeZhang closed 3 years ago

XinzeZhang commented 3 years ago

Hi, stefanonardo.

In your src file 'torchesn/nn/echo_state_network.py', at the line of 248: ''' if torch.det(A) != 0: ''' The thought of using the determinant to judge whether the left partial number A has an inverse is right. However, since the results of det(A) can be really small but not equal to 0, the above implementation can treat this situation with equal to 0.

An improvement can be: ''' col = X.size(1) orig_rank = torch.matrix_rank(A).item() tag = 'Inverse' if orig_rank == col else 'Pseudo-inverse'

if tag == 'Inverse': W = torch.mm(torch.inverse(A), self.XTy).t() else: W = torch.mm(torch.pinverse(A), self.XTy).t() '''

XinzeZhang commented 3 years ago

I found this problem in my own work and this risky judgment could lead to some serious calculation errors in some real-world scenarios.

stefanonardo commented 3 years ago

Hi Xinze, thank you for your suggestion, I find it very reasonable! Please create a PR.

XinzeZhang commented 3 years ago

Hi Stefanonardo, the PR has been created in https://github.com/stefanonardo/pytorch-esn/pull/11#issue-697031140