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

Refactor NPO #1189

Merged
merged 9 commits into from
Mar 10, 2020
Merged

Refactor NPO #1189

merged 9 commits into from
Mar 10, 2020

Conversation

ahtsan
Copy link
Contributor

@ahtsan ahtsan commented Mar 3, 2020

This PR refactors NPO, as discussed in #1172 , in preparation of RL2NPO.

@ahtsan ahtsan requested review from ryanjulian, krzentner and a team March 3, 2020 21:12
@ahtsan ahtsan requested a review from a team as a code owner March 3, 2020 21:12
@ghost ghost removed their request for review March 3, 2020 21:12

Returns:
numpy.ndarray: Stacked and padded tensor. Shape: :math:`(N, D, S^*)`
where K is the len of input paths.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

K?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a typo, fixed

@ryanjulian
Copy link
Member

Can you describe in your commit message/subject in a little more detail what and why you are refactoring NPO?

@ahtsan ahtsan requested a review from a team March 3, 2020 21:55
@ghost ghost requested review from nish21 and removed request for a team March 3, 2020 21:55
@ahtsan
Copy link
Contributor Author

ahtsan commented Mar 3, 2020

Can you describe in your commit message/subject in a little more detail what and why you are refactoring NPO?

Done

@codecov
Copy link

codecov bot commented Mar 3, 2020

Codecov Report

Merging #1189 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1189      +/-   ##
==========================================
- Coverage   87.99%   87.97%   -0.02%     
==========================================
  Files         184      184              
  Lines        8771     8776       +5     
  Branches     1108     1111       +3     
==========================================
+ Hits         7718     7721       +3     
- Misses        854      855       +1     
- Partials      199      200       +1     
Impacted Files Coverage Δ
src/garage/tf/policies/gaussian_gru_policy.py 97.59% <ø> (ø)
src/garage/misc/tensor_utils.py 93.87% <100.00%> (+0.61%) ⬆️
src/garage/tf/algos/npo.py 95.21% <100.00%> (-0.08%) ⬇️
.../exploration_strategies/epsilon_greedy_strategy.py 96.29% <0.00%> (-3.71%) ⬇️
src/garage/tf/misc/tensor_utils.py 75.37% <0.00%> (-0.75%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c61253c...a61c16f. Read the comment docs.

@ahtsan ahtsan force-pushed the refactor_npo branch 2 times, most recently from be4f360 to 8417901 Compare March 4, 2020 18:25
@@ -23,20 +23,26 @@ def discount_cumsum(x, discount):
axis=0)[::-1]


def explained_variance_1d(ypred, y):
def explained_variance_1d(ypred, y, valids):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm perhaps valids should be optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea

@ahtsan ahtsan force-pushed the refactor_npo branch 3 times, most recently from 9a275f4 to 4c4ec69 Compare March 7, 2020 01:34
# pylint: disable=abstract-class-instantiated, no-member
# This test cause low memory with some reason
@pytest.mark.flaky
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would be really surprising. you seen this in the wild on master?

It seems likely that some other part of the test suite is using all if your memory, and this test just became a victim.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense, I will remove the decorator

@ryanjulian ryanjulian removed the request for review from nish21 March 9, 2020 21:18
* Rename fit_baseline to fit_baseline_with_data to avoid naming
conflicts with argument names in RL2NPO
* Unify returns and rewards in sample_data to dense form
Copy link
Contributor

@krzentner krzentner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor question, but LGTM.

@ahtsan ahtsan merged commit f3a32e7 into master Mar 10, 2020
@ahtsan ahtsan deleted the refactor_npo branch March 10, 2020 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants