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

Disable the done signal. #45

Merged
merged 1 commit into from
Mar 31, 2020
Merged

Disable the done signal. #45

merged 1 commit into from
Mar 31, 2020

Conversation

krzentner
Copy link
Contributor

This fixes #43 in what I think is the cleanest way possible. There should probably be more discussion before we officially make a v2, but this might be of use to some people immediately.

This is definitely of interest to @avnishn and @lywong92 .

@krzentner
Copy link
Contributor Author

Actually, this change is still largely incomplete, since I need to update all of the environment dictionaries. I'll probably have time to do that in a week's time.

@krzentner krzentner changed the title Make done=False if version=v2 Aloow disabling the done signal. Mar 4, 2020
@krzentner
Copy link
Contributor Author

krzentner commented Mar 4, 2020

I believe there was some discussion that came to the conclusion that adding this option is fine?

If so, I believe this change can be merged in its current state.

@ryanjulian
Copy link
Contributor

I think this could be made simpler by simply disabling it universally. We should also raise an exception in each env if someone steps past 150.

@krzentner
Copy link
Contributor Author

I think this could be made simpler by simply disabling it universally.

Perhaps? I can certainly imagine someone wanting to be able to easily preserve the old behavior.

We should also raise an exception in each env if someone steps past 150.

I added that now. Since we probably want that check to be done in a base class method, I don't think there's much more simplicity to be had.

@krzentner krzentner changed the title Aloow disabling the done signal. Allow disabling the done signal. Mar 4, 2020
@ryanjulian
Copy link
Contributor

I guess the missing context is that I discussed this with the authors last week and we decided that the simplest thing to do is to never send a done signal. This is consistent with the CoRL publication, and the current version is inconsistent.

Those needing a time limit implemented via termination (e.g. some on-policy implementations) will have to add it themselves.

I apologize for not specifically communicating about this (one of those many TODOs which never got visited).

@krzentner krzentner force-pushed the v2_remove_done_signal branch 3 times, most recently from 0b28e1e to 0f40380 Compare March 5, 2020 01:45
@krzentner krzentner changed the title Allow disabling the done signal. Disable the done signal. Mar 5, 2020
@krzentner krzentner force-pushed the v2_remove_done_signal branch 2 times, most recently from 477ce48 to e47607a Compare March 5, 2020 02:59
@ryanjulian ryanjulian added this to the v1.1 milestone Mar 5, 2020
Also, throw an exception if the done signal is ignored.
@ryanjulian
Copy link
Contributor

@krzentner is this ready to merge?

@krzentner
Copy link
Contributor Author

@krzentner is this ready to merge?

Yes. It leaves the path length alone (150 in some envs, 200 in others), removes the done signal, and throws an exception when the path length is exceeded.

@ryanjulian ryanjulian merged commit e31ca5d into master Mar 31, 2020
@ryanjulian ryanjulian deleted the v2_remove_done_signal branch March 31, 2020 00:00
bryanoliveira added a commit to bryanoliveira/metaworld that referenced this pull request Apr 1, 2020
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.

All metaworld environments report done=True at max time steps.
2 participants