Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incompatibility with up-to-date pytorch and numpy #4

Closed
dreamer2368 opened this issue Jun 26, 2024 · 2 comments
Closed

Incompatibility with up-to-date pytorch and numpy #4

dreamer2368 opened this issue Jun 26, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@dreamer2368
Copy link
Collaborator

dreamer2368 commented Jun 26, 2024

This issue corresponds to the legacy code. The work-in-progress python package has already applied the fix for this issue.
The legacy code currently solves SINDY using the outdated torch/numpy routines at two routines in BurgersEqn1D/utils.py:

  1. find_sindy_coef uses torch.pinverse
  2. solve_sindy uses np.linalg.lstsq

Other examples are not examined, but expected to use the same routines.

For torch>=2.3.0 and numpy>=1.26.4, the internal behaviors of torch.pinverse and numpy.linalg.lstsq have been changed, which make them unstable for our use of sindy loss training. Earlier versions of torch or numpy may have the same issue.

Currently the solution is:

  1. Use torch.linalg.lstsq insteady of torch.pinverse. This is also recommended from pytorch official documentation.
  2. It is not clear numpy.linalg.lstsq is unstable. However, it often returns different results than torch.linalg.lstsq, given the same system. numpy official documentation says that if there are multiple solutions, the solution with minimal norm is chosen, which could be a reason for this inconsistency. For the best of our practice, it is recommended to use the same torch.linalg.lstsq.
@dreamer2368 dreamer2368 added the bug Something isn't working label Jun 26, 2024
@CBonneville45
Copy link
Collaborator

CBonneville45 commented Jun 26, 2024 via email

@dreamer2368
Copy link
Collaborator Author

As of PR #8 , the legacy code that has this version inconsistency is moved to legacy directory. The package source directory src/lasdi has fixed this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants