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 listener pattern to SyncLearning #8050

Merged
merged 2 commits into from Aug 2, 2019

Conversation

@aboulang2002
Copy link
Contributor

commented Jul 26, 2019

Signed-off-by: Alexandre Boulanger aboulang2002@yahoo.com

What changes were proposed in this pull request?

The goal of this PR is to remove IDataManager from SyncLearning. I added a listener pattern to SyncLearning, and a listener class (DataManagerSyncTrainingListener) that will make the calls to DataManager that SyncLearning used to do.

I removed the try/catch from SyncLearning.train(). Listeners should be well-behaved and not throw. If a listener need to stop the training (because of an IOException for example), it can with SyncTrainingEvent.setCanContinue(false).

Since SyncLearning doesn't use IDataManager anymore, I moved it to ASyncLearning (where a similar refac will be done in an upcoming PR).

I did not add javadoc to IDataManager and IAsyncGlobal.
In the case of IDataManager/DataManager, it may be worthwhile to refac it. For example, isSaveData() will soon not be necessary anymore. Also, it seems strange to me that a DataManager would return the path where to save the videos which are then saved by a HistoryProcessor. I will write the javadoc when I change this class.
In the case of IAsyncGlobal, I don't understand a few things and I'd like to take the time to read and understand "Hogwild!" before writing the javadoc.

Summary of changes:

  • Moved up IDataManager from Learning to ASyncLearning and removed it from SyncLearning
  • Some code in QLearningDiscrete.postEpoch() has been moved to DataManagerSyncTrainingListener.onEpochEnd()
  • Added listener pattern to SyncLearning along with a listener, DataManagerSyncTrainingListener.
  • Adjusted SyncLearningTest for the new listener
  • Added refac tests to QLearningDiscrete to make sure it remains functionally the same as before.

How was this patch tested?

Unit tests

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 listener pattern to SyncLearning
Signed-off-by: Alexandre Boulanger <aboulang2002@yahoo.com>

@aboulang2002 aboulang2002 marked this pull request as ready for review Jul 26, 2019

@AlexDBlack
Copy link
Contributor

left a comment

I only quickly scanned this, looks reasonable at first glance. Get rid of the e.printStackTrace() and use logging only instead.
Missing javadoc in a bunch of places.

Did requested changes
Signed-off-by: Alexandre Boulanger <aboulang2002@yahoo.com>
@aboulang2002

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Summary of changes:

  • Added/changed javadocs
  • Make listeners return a response instead of changing a canContinue property (also changed SyncLearning and unit tests)
  • removed all e.printStackTrace() in DataManagerSyncTrainingListener
@saudet
saudet approved these changes Aug 1, 2019
@saudet

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Looks good to me! Tested it out a bit, and it doesn't appear to break anything.

@AlexDBlack
Copy link
Contributor

left a comment

LGTM 👍

@AlexDBlack AlexDBlack merged commit b2145ca into eclipse:master Aug 2, 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_synctraininglistener branch Aug 2, 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.