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

Removed deprecated torch.lstsq, replaced by torch.linalg.lstsq #33

Merged

Conversation

raphaelattias
Copy link
Contributor

@raphaelattias raphaelattias commented Jan 11, 2023

As of September 22, torch.lstsq is deprecated (pytorch/pytorch#70980) and has been replaced by torch.linalg.lstsq. This PR solves an issue for Pytorch version 1.13.

Copy link
Collaborator

@andreped andreped left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

.vscode/launch.json Outdated Show resolved Hide resolved
example.py Outdated Show resolved Hide resolved
@@ -49,7 +49,7 @@ def __find_concentration(self, OD, HE):
Y = OD.T

# determine concentrations of the individual stains
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I am not sure that doing this is backward compatible, as I believe torch.linalg.lstsq does not exist in older versions of torch. You will need to find a fix that works for all versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the Torch repo that change was made in Torch 1.9. I suggest bumping the minimum version of Torch required from 1.8 to 1.9 to assure backward compatibility:

https://github.com/pytorch/pytorch/blob/b0c86caa1d46a16195682e2afe5456f97265aa53/torch/_linalg_utils.py#L111

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are surprisingly many people that use older versions of pytorch and tf. Also, 1.9 isn't really that old.

I think a good fix for this is to check which pytorch version that is used and introduce a if statement to handle older versions. You can get the torch version by:

import torch
print(torch.__version__)

@@ -73,7 +73,7 @@ jobs:
matrix:
os: [ windows-2019, ubuntu-18.04, macos-11 ]
python-version: [ 3.6, 3.7, 3.8, 3.9 ]
pytorch-version: [1.8.0, 1.9.0, 1.10.0, 1.11.0, 1.12.0]
pytorch-version: [1.9.0, 1.10.0, 1.11.0, 1.12.0, 1.13.0]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sneaky fix ;) But it would be better if we also supported older versions of pytorch (< 1.9).

@@ -73,7 +73,7 @@ jobs:
matrix:
os: [ windows-2019, ubuntu-18.04, macos-11 ]
python-version: [ 3.6, 3.7, 3.8, 3.9 ]
pytorch-version: [1.9.0, 1.10.0, 1.11.0, 1.12.0, 1.13.0]
pytorch-version: [1.8.0, 1.9.0, 1.10.0, 1.11.0, 1.12.0, 1.13.0]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Just handling new/older torch version in the repo that remains :]

Copy link
Collaborator

@andreped andreped left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

@@ -49,7 +49,11 @@ def __find_concentration(self, OD, HE):
Y = OD.T

# determine concentrations of the individual stains
return torch.linalg.lstsq(HE, Y)[0]

if torch.__version__ >= (1,9,0):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! It is a little suboptimal to have this if statement being called every single time this method is called. Might be better to do this only once in the init() or similar. And then, this method knows which lstsq to use, but we can fix that in the near future. LGTM.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it should be a rather quick fix to do (in init or in the file header)

@andreped
Copy link
Collaborator

Note that, as I'm not a maintainer of this project, @carloalbertobarbano is needed to:

  1. Approve running workflows such that we can verify that all unit tests are successful.
  2. To accept the PR or not.

He will probably return from holiday anytime soon, @raphaelattias :]

@carloalbertobarbano carloalbertobarbano changed the base branch from main to development January 17, 2023 09:00
@carloalbertobarbano
Copy link
Member

Hi @raphaelattias, I finally came back from vacation. Thanks for your contribution! I will happily merge it once you refactor the torch version check

@andreped
Copy link
Collaborator

Hi @raphaelattias, I finally came back from vacation. Thanks for your contribution! I will happily merge it once you refactor the torch version check

I believe that was already done., I just did not see that in one of my latest comments. See this commit: c5b715c

Correct me if I'm wrong. Current version is here, and contains the torch version check:
https://github.com/raphaelattias/torchstain/blob/deprecated-lstsq/torchstain/torch/normalizers/macenko.py#L53

@carloalbertobarbano
Copy link
Member

Yes, but I would like the version check to be moved from __find_concentration to either __init__ or in the file header.

@andreped
Copy link
Collaborator

Oh yeah, sure. I guess you will make that final change yourself in the development branch. Not sure @raphaelattias is available to make that fix right now.

@raphaelattias
Copy link
Contributor Author

Hi @andreped thank you for the feedback, I am working on it :)

Updated eigen vectors solver
@carloalbertobarbano
Copy link
Member

Thanks for the cooperation @raphaelattias

@carloalbertobarbano carloalbertobarbano merged commit 64622cd into EIDOSLAB:development Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants