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

ENH: Adds pytorch 0.4 support #185

Merged
merged 18 commits into from
May 2, 2018
Merged

Conversation

thomasjpfan
Copy link
Member

Closes #184

@ottonemo ottonemo added this to Ready for review in Release 0.2 Apr 26, 2018
@ottonemo ottonemo moved this from Ready for review to In progress in Release 0.2 Apr 26, 2018
# currently, I see no solution that would result in a 1-dim
# LongTensor after passing through torch's DataLoader. If
# there is, we should use that instead. Otherwise, this will
# be obsolete once pytorch scalars arrive.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is still valid though. This is a workaround for the case where you pass a tensor of shape (n,) as y to the data loader which will result in y.shape = (n,1) which we correct to its original shape (n,) here.

You can verify for yourself that this is still the case:

X = torch.zeros(10, 5)
y = torch.zeros(10)
d = TensorDataset(X, y)
l = DataLoader(d)
Xi, yi = next(iter(l))
assert len(yi.size()) == 1 # fails, is 2

Copy link
Contributor

Choose a reason for hiding this comment

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

For me it's 1!

Copy link
Member Author

Choose a reason for hiding this comment

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

len(yi.size()) is 1 in pytorch 0.3.1 and 0.4.

Copy link
Member

@ottonemo ottonemo Apr 27, 2018

Choose a reason for hiding this comment

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

True, the problem lies with our to_tensor. We'll see what we can do.

@benjamin-work benjamin-work moved this from In progress to Ready for review in Release 0.2 Apr 27, 2018
@githubnemo
Copy link
Contributor

@BenjaminBossan I think this PR is ready. Do you think we can merge this so at least the master branch works with pytorch 0.4.0 until we release the next version?

@BenjaminBossan
Copy link
Collaborator

I can't merge before Wednesday ;) There are some references to Variables still left that could be removed, as well as a typo in your deprecation warning message, but that can be fixed later.

@benjamin-work
Copy link
Contributor

@ottonemo I fixed the points I addressed earlier. If it's fine by you, I would merge now.

@ottonemo ottonemo merged commit f91b60a into skorch-dev:master May 2, 2018
@ottonemo ottonemo moved this from Ready for review to Done in Release 0.2 May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Release 0.2
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants