duckietown / gym-duckietown

Self-driving car simulator for the Duckietown universe
http://duckietown.org
Other
51 stars 19 forks source link

new wrapper #78

Closed nithin127 closed 6 years ago

nithin127 commented 6 years ago

to resize images as 80x80

maximecb commented 6 years ago

This is a good idea, but there are two things I would do differently:

nithin127 commented 6 years ago

So, import cv2 would be called every time the env.reset() or env.step() is called; should I go ahead with this?

maximecb commented 6 years ago

Yes, put the import inside both reset and step.

maximecb commented 6 years ago

Don't add the RewardWrapper to the repo. It's clearly specific to your own use-case/experiments.

nithin127 commented 6 years ago

done;

bhairavmehta95 commented 6 years ago

Why not just use the Pytorch transforms for this?

bhairavmehta95 commented 6 years ago
resize = T.Compose([T.ToPILImage(),
                    T.Resize(40, interpolation=Image.CUBIC),
                    T.ToTensor()])

https://pytorch.org/tutorials/intermediate/reinforcement_q_learning.html

(You'd put 80 instead of 40)

Also, looking through this, only axes 0 and 2 are swapped, which would make your image, CxWxH instead of CxHxW, no?

https://pytorch.org/docs/stable/torchvision/transforms.html#torchvision.transforms.ToPILImage

nithin127 commented 6 years ago

@bhairavmehta95 Florian suggested that we try 80; And I swap(0,2) just to resize it, then I swap it back -- (0,2) again; so essentially the Resize wrapper is not changing any dimensions

But the PyTorchObsWrapper does use an observation.transpose(2, 1, 0); do you think it should be observation.transpose(2, 0, 1)

bhairavmehta95 commented 6 years ago

Yeah dimensions don't matter, so I think he's right.

I don't think it matters, as long as it's consistent. Only reason I'd say 2, 0, 1 is then you're only moving the channel. Makes it better to visualize, otherwise, if you feed your image tensor into something like tensorboardX, it will show up rotated. (As it's expecting 2, 0, 1, but you're giving it 2, 1, 0)

nithin127 commented 6 years ago

done!

maximecb commented 6 years ago

The new wrapper is fine, but you've made a one-line change to the PyTorchObsWrapper. Can you remove that?