maciejkula / spotlight

Deep recommender models using PyTorch.
MIT License
2.97k stars 421 forks source link

fixing a 0-dim tensor to python warning to prevent a break come PyTorch 0.5. Pertinent to ExplicitFactorizationModel and ImplicitFactorizationModel. #111

Closed nikisix closed 6 years ago

nikisix commented 6 years ago

I'm on OSX and installed with pip. Which I attribute to the discrepancy. Guess we'll see if this is a pip/conda issue come PyTorch 0.5

maciejkula commented 6 years ago

You haven't updated the version used for tests in the pull request: CI still uses PyTorch v0.3.0.

nikisix commented 6 years ago

@maciejkula Not quite sure how to do that, but I would be amenable of course. Should've mentioned this is on PyTorch v0.4.0

maciejkula commented 6 years ago

I'm afraid that in that case this will turn into a bigger project of making Spotlight PyTorch v0.4.0 compatible :)

As the first step you need to change:

nikisix commented 6 years ago

I see, would you rather I close this for now then?

maciejkula commented 6 years ago

That depends! I don't know how much we actually need to change, and I don't know how keen you are on doing some of the work. If you have time, and are looking for an issue to work on, I'd be happy to collaborate with you on that! Otherwise, I'll get round to this myself in the next couple of weeks.

nikisix commented 6 years ago

Always happy to pitch in :)

Attached are the tests run on the master branch and this branch in the 0-dim_tensor_bugfix branch. It would appear the commits in this branch are in the right direction.

I'm not totally sure what's going on with the

  assert mrr.mean() > expected_mrr

test in test_implicit_lstm_synthetic, but maybe changing the threshold to 0.60 would avoid some assertion errors I'm seeing, if that's not too crazy at the conceptual level.

Other than that, the only other thing I'm seeing would be the following warning:

tests/factorization/test_implicit.py::test_adaptive_hinge /usr/local/lib/python2.7/site-packages/torch/tensor.py:255: UserWarning: non-inplace resize is deprecated warnings.warn("non-inplace resize is deprecated")

test-0dim.txt test-master.txt

nikisix commented 6 years ago

After taking a quick peek for resizes, these guys may be the offenders

spotlight/factorization/implicit.py
276:                                                            .resize(batch_size, 1)
278:                                                            .resize(batch_size * n))

spotlight/sequence/representations.py
550:        user_representations = user_representations.resize(batch_size,
maciejkula commented 6 years ago

If you push the requirements changes to this PR we'll be able to see what needs fixing in the CI logs.

nikisix commented 6 years ago

Ok, looks like you ran the upgrade yesterday. I'll close this out then.

maciejkula commented 6 years ago

Yes, sorry --- I got some time over the weekend.