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

RL4J - VideoRecorder #8106

Merged
merged 3 commits into from Sep 30, 2019
Merged

RL4J - VideoRecorder #8106

merged 3 commits into from Sep 30, 2019

Conversation

@aboulang2002
Copy link
Contributor

aboulang2002 commented Aug 16, 2019

What changes were proposed in this pull request?

Currently in RL4J, the HistoryProcessor can be configured to capture a video of the input. On the conceptual side, a 'HistoryProcessor' shouldn't be responsible of handling the video creation. Also, when we'll start using the DataSet for observations, the HistoryProcessor will receive observations that are transformed to be used by the Learning classes instead of the raw observations of the game.

In this PR, I isolated the video recording code in a simple class, the VideoRecorder.

How was this patch tested?

Manual Tests. Can it be unit tested?

Quick checklist

The following checklist helps ensure your PR is complete:

  • Eclipse Contributor Agreement signed, and signed commits - see IP Requirements page for details
  • Reviewed the Contributing Guidelines and followed the steps within.
  • Created tests for any significant new code additions.
  • Relevant tests for your changes are passing.
Signed-off-by: unknown <aboulang2002@yahoo.com>
Signed-off-by: unknown <aboulang2002@yahoo.com>
@saudet

This comment has been minimized.

Copy link
Member

saudet commented Aug 18, 2019

Looks good, but I think we could put it in the relevant places right away to actually record images?

@aboulang2002

This comment has been minimized.

Copy link
Contributor Author

aboulang2002 commented Aug 18, 2019

That's the thing; video recording doesn't have a place anymore since I'll be removing/deprecating it from the HistoryProcessor. It should go somewhere in the MDP, but the MDP is only an interface in RL4J; it's up to the user to implement it.
Maybe we should add a base class for MDP for everything that is video/image-sequence based?

@saudet

This comment has been minimized.

Copy link
Member

saudet commented Aug 19, 2019

I'd kind of see it along with the rest of the logging stuff, around DataManager maybe?

@aboulang2002 aboulang2002 changed the title RL4J - VideoRecorder [WIP] RL4J - VideoRecorder Aug 19, 2019
@aboulang2002

This comment has been minimized.

Copy link
Contributor Author

aboulang2002 commented Aug 19, 2019

The file is located in the same directory as DataManager. Is it what you meant?

WIP: I'd like to change the MDPs in ALE, GYM, VizDoom and Malmo to use a base ImageSequenceMDP that will handle the video recording.

@saudet

This comment has been minimized.

Copy link
Member

saudet commented Aug 20, 2019

I mean, the DataManager contains the path where the videos are going to be recorded, so it makes sense if that one does the recording too. I doesn't feel like it's something that the environment should be responsible for.

@aboulang2002

This comment has been minimized.

Copy link
Contributor Author

aboulang2002 commented Aug 21, 2019

I agree, it does not belong to the environment. But, remember that the learning classes will receive a DataSet with all the transforms already applied, and that's what the DataManager has access to. Plus, I don't like the fact that the DataManager tries to do a lot of things.

What I had in mind is that something will have to handle the observations given by the MDP (a byte array) and convert it to a DataSet. As you know, that byte array can represent anything like a video feed or the positions of pieces on a gaming board. I'm not 100% sure how to do that elegantly. It's easy if what you have is a single video feed or a 1D signal, but I'd like RL4J to be able to easily support advanced cases such as multiple video, audio or sensor feeds all at once. Anyways, for now, what I had in mind is introduce a handler (interface) between the MDP and Learning do the conversion and implement a video handler that can also records the video.

What do you think?

I'll be away for a few days but I'll work on this when I return.

@saudet

This comment has been minimized.

Copy link
Member

saudet commented Aug 22, 2019

I see what you mean. We need access to data before transformation, so the environment has to provide some information about that explicitly. Sounds good, I'll leave you think of something! :)

Signed-off-by: unknown <aboulang2002@yahoo.com>
@aboulang2002

This comment has been minimized.

Copy link
Contributor Author

aboulang2002 commented Sep 23, 2019

I changed HistoryProcessor to use VideoRecorder. As I wrote in a previous comment, I think its place will be in the class that will handle observations but, for now, the goal of this PR is just to introduce VideoRecorder.

@aboulang2002 aboulang2002 changed the title [WIP] RL4J - VideoRecorder RL4J - VideoRecorder Sep 23, 2019
@saudet saudet self-requested a review Sep 30, 2019
@saudet
saudet approved these changes Sep 30, 2019
@saudet saudet merged commit d5e98af into eclipse:master Sep 30, 2019
1 check passed
1 check passed
eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details
@aboulang2002 aboulang2002 deleted the aboulang2002:ab2002_rl4j_video_recorder branch Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.