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 Added IDataManager #8034

Merged
merged 1 commit into from Jul 25, 2019

Conversation

@aboulang2002
Copy link
Contributor

commented Jul 20, 2019

What changes were proposed in this pull request?

Classes in RL4J directly uses the implementation of DataManager. In this PR, I add an interface (IDataManager) and made changes to a few classes in order to use this interface instead of DataManager.

What I have in mind (in an upcoming PR) is to remove DataManager as a dependency and use a listener pattern in the classes that currently uses DataManager. This way we move a bit closer to the single responsibility principle and there will be a generic way to listen to the beginning and end of training epochs.

The goal of this PR is to add unit tests to AsyncThread and SyncLearning to be sure that I don't break anything when I add the listener pattern. Also, I added an interface to AsyncGlobal to be able to create these unit tests.

Summary of changes:

  • Added IDataManager and made other classes use this instead of DataManager
  • Moved interface StatEntry from DataManager to IDataManager
  • Added interface IAsyncGlobal
  • Added tests to SyncLearning and AsyncThread

How was this patch tested?

Unit tests to AsyncThread and SyncLearning

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.
Added interface IDataManager
Signed-off-by: Alexandre Boulanger <aboulang2002@yahoo.com>
@saudet

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

I've tested that it doesn't break anything, so looks good to me. @AlexDBlack ?

@saudet saudet requested a review from AlexDBlack Jul 22, 2019

@AlexDBlack
Copy link
Contributor

left a comment

Looks fine to me, only thing that stands out is the lack of javadoc on IDataManager and IAsyncGlobal, which isn't the end of the world by any means, but would be nice to have...

@aboulang2002

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

You can merge this PR, I will add proper javadoc in my next PR. Thanks @saudet and @AlexDBlack.

@AlexDBlack AlexDBlack merged commit 87d2b2c into eclipse:master Jul 25, 2019

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_datamanager branch Jul 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.