make size param of Scale (h,w) not (w,h) #256
Conversation
There was a problem hiding this comment.
I think this is a necessary change (the previous behaviour seems like a bug to me), but as this is a breaking change I think we need to merge it just before the new release of torchvision, and explicitly mention this breaking change in the release notes.
@soumith do you have better ideas?
|
i think introducing backwards-compatible ordering for Scale as a one-time thing is fine, if it makes things consistent across board. |
|
That's a good point.
Let's go for the (H, W), to make it more consistent with the image ordering (and also it makes it easier to get the dimensions right, e.g., @alykhantejani what do you think? Could you also change that? |
@soumith did you mean a backwards incompatible change to If so, I will update the PR to keep it (h,w) and make the breaking change to |
|
yes, go ahead. |
|
👍 I've made these changes to scale. |
|
LGTM! Thanks Aly! |
fmassa
changed the title
* integrate alias table * fix for negatives * changes to the main ncf file * fixes * more fixes * fix for crash * remove print statements * address comments * fixes * fixes * Fixes minor bugs and cleans up conversion program. generate_negatives() now takes a user array, to allow for better compatibility with the training program ncf.py. * Fixes bugs in ncf.py and aligns cache reading scheme with convert.py * convert.py -- Removes unnecessary import. * convert.py - Removing extraneous assertion. * Fixes expression for length of list. * More bug fixes, found by generating a larger data set with 'holes' in it. The overall takeaway for future readers is the necessity of carefully noting whether a data structure is indexed by the raw user ids or by the alias table set of id's (iota). * Removes the generator iter_fn * Store alias table data in 32-bit quantities. * Temporary commit towards 32-bit. * Allow neg generation using alias cached sampler. * Rescinds the 32-bit change. This is not necessary to run. OOMs were due to GPU dataloader, as opposed to CPU * Fixes bug where users were replicated before negative generation. Train negatives were not being concatenated, leading to extra work being done. Also, minor code cleanup. * Batched and multi-threaded negative generation. About 3 mins with 64 threads. * Code cleanup before merge. * This commit converges on ml-20mx1x1. Roll forward convergence fix for single-threaded negative generation. Error was in generate_negatives() assumptions. Negatives for each user must be contiguous. torch.repeat() does not do the right thing, but numpy.repeat() does. * Roll forward convergence fixes to multi-threaded version. Change default to call multi-threaded version. * Make convert.py deterministic. * Delete run.sh * Revert "Roll forward convergence fixes to multi-threaded version. Change default to call multi-threaded version." This reverts commit aee0359cdce5524bdcdf8c9d74061c6b5599adea. * For data conversion, it is necessary to use the libraries at these specified versions. Experiments show that using HEAD versions of these libraries lead to different data being emitted and downstream convergence issues. * Update readme instructions for ml-1b integration. * Further README updates. * Code cleanup before master merge. * Update recommendation README to remove ml-20m reference.
Note: this PR introduces non backwards compatible changes
Currently, there is some inconsistency in the transforms. For example, the
Scaletransform takes in asizeparam that can be a sequence and is expected to be in format(w,h), but theCenterCropandRandomCroptransforms expectsizeto be in(h,w).Furthermore,
PIL'sImageclass also expects size tuples to be in the format(w, h). We should standardize this across the transforms, and should probably use(w, h)as everything it PIL powered.This PR changes the expected format of the
sizeparam inRandomCropandCenterCrophowever this change is not backward compatible.An alternative to this would be to make the transforms explicitly accept a
wandhparam in the constructor, keeping thesizeparam and adding deprecation warnings - however the inconsistency will still exist until we fully remove size in this case.