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.

Sign up for GitHub

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

Jump to bottom

Move garage.misc.instrument to garage.experiment #386

Merged
merged 1 commit into from
Nov 27, 2018
Merged

Move garage.misc.instrument to garage.experiment #386

merged 1 commit into from
Nov 27, 2018

Conversation

jonashen
Copy link
Member

@jonashen jonashen commented Nov 5, 2018

Fixes: #383.

@jonashen jonashen self-assigned this Nov 5, 2018
@jonashen jonashen requested review from ryanjulian, CatherineSue and a user November 5, 2018 22:28
@jonashen jonashen requested a review from a team as a code owner November 5, 2018 22:28
@codecov
Copy link

codecov bot commented Nov 5, 2018 •
edited
Loading

Codecov Report

Merging #386 into master will increase coverage by 0.04%.
The diff coverage is 38.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #386      +/-   ##
==========================================
+ Coverage   64.91%   64.96%   +0.04%     
==========================================
  Files         213      214       +1     
  Lines       13834    13839       +5     
==========================================
+ Hits         8981     8991      +10     
+ Misses       4853     4848       -5
Impacted Files Coverage Δ
garage/experiment/experiment.py 20.92% <0%> (ø)
garage/experiment/__init__.py 100% <100%> (ø)
.../theano/optimizers/conjugate_gradient_optimizer.py 74.05% <0%> (+0.63%) ⬆️
garage/envs/mujoco/gather/gather_env.py 57.57% <0%> (+1.01%) ⬆️
garage/theano/optimizers/first_order_optimizer.py 85.5% <0%> (+1.44%) ⬆️

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 6837bca...b23b3f9. Read the comment docs.

@coveralls
Copy link

coveralls commented Nov 5, 2018 •
edited
Loading

Coverage Status

Coverage decreased (-0.03%) to 65.338% when pulling b23b3f9 on garage_experiment into 6837bca on master.

@ryanjulian
Copy link
Member

I think it is arguable that nb_utils belongs here (either here or in garage.logger).

Please update #358 once you're done.

@jonashen jonashen force-pushed the garage_experiment branch 2 times, most recently from 2f661e7 to a56f949 Compare November 6, 2018 18:57
@jonashen
Copy link
Member Author

jonashen commented Nov 7, 2018

If nb_utils is not used anywhere in garage, should I just delete the file or move it to garage.experiment.nb_utils

ryanjulian
ryanjulian reviewed Nov 12, 2018
View reviewed changes
garage/experiment/__init__.py Outdated
from garage.experiment.experiment import VariantGenerator

__all__ = [
"concretize",
Copy link
Member

Choose a reason for hiding this comment

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

Is concretize called from anywhere except inside this package? If not, it's not part of the API and doesn't need to be in __all__. Ditto for the other modules.

Copy link
Member

Choose a reason for hiding this comment

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

Note that tests don't count since they can access private APIs.

ryanjulian
ryanjulian reviewed Nov 12, 2018
View reviewed changes
garage/experiment/experiment.py Outdated
exp_prefix.replace("_", "-") + \
"/" + \
task["exp_name"]
exp_prefix.replace("_", "-") + "/" + task["exp_name"] # noqa: E122
Copy link
Member

Choose a reason for hiding this comment

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

Uh is there a reason we can't just fix all these?

ryanjulian
ryanjulian reviewed Nov 12, 2018
View reviewed changes
scripts/run_experiment.py Outdated
@@ -226,14 +226,15 @@ def child_proc_shutdown(children):
for child in alive:
error_msg += (str(
child.as_dict(
attrs=["ppid", "pid", "name", "status", "cmdline"])) +
"\n")
attrs=["ppid", "pid", "name", "status", "cmdline"])) + "\n"
Copy link
Member

Choose a reason for hiding this comment

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

Why not just fix?

@ryanjulian
Copy link
Member

LMK when this is ready for review again.

Please just fix the PEP8 bugs.

@jonashen jonashen force-pushed the garage_experiment branch 2 times, most recently from ab0e50e to 253a521 Compare November 13, 2018 22:13
@ryanjulian
Copy link
Member

ryanjulian commented Nov 19, 2018 •
edited
Loading

What is the blocker here? Still the PEP8 conflicts?

Did you have any luck with modifying the YAPF config?

@jonashen
Copy link
Member Author

Removed PEP8 conflicts. Ready for another check.

@jonashen jonashen merged commit d5e350b into master Nov 27, 2018
@jonashen jonashen deleted the garage_experiment branch November 27, 2018 18:46
@ryanjulian ryanjulian mentioned this pull request Feb 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanjulian ryanjulian ryanjulian approved these changes

@CatherineSue CatherineSue CatherineSue approved these changes

Assignees

@jonashen jonashen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jonashen @coveralls @ryanjulian @CatherineSue