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

Don't compute observations in the final state #59

Closed
gasse opened this issue Jun 2, 2020 · 5 comments · Fixed by #206
Closed

Don't compute observations in the final state #59

gasse opened this issue Jun 2, 2020 · 5 comments · Fixed by #206
Labels
type/enhancement 🚀 New feature or request

Comments

@gasse
Copy link
Member

gasse commented Jun 2, 2020

Describe the problem of improvement suggested

Environments should not ask for an observation in the final state (when done == True). This observation is most likely impossible to compute, since SCIP will likely not be in a SOLVING state. And the final observation of an episode is not relevant for learning anyway. That way, Observation functions do not deal with that situation, and silently return empty observations (current behaviour).

Describe the solution you'd like

The Environment does not call the registered ObservationFunction when done==True, and the ObservationFunctions do not check any more for SCIP_STAGE_SOLVING.

Describe alternatives you've considered

na

Additional context

na

@gasse gasse added the type/enhancement 🚀 New feature or request label Jun 2, 2020
@gasse gasse added this to the v0.2.0 milestone Jun 2, 2020
@gasse gasse changed the title Dont compute observations in the finale state Dont compute observations in the final state Jun 2, 2020
@gasse gasse changed the title Dont compute observations in the final state Don't compute observations in the final state Jun 2, 2020
@AntoinePrv
Copy link
Member

I keep hesitating back forth whether or not we want to do this.

On the one hand if someone want the observation function to be called on terminal states, this makes it impossible.
On the other end, I cannot think of observation that could be extracted on terminal states...

@gasse
Copy link
Member Author

gasse commented Jun 17, 2020

I agree, this disables from the start any possibility for an observation to be returned in the final state. Still, I do not see a potential reason for wanting that final observation. The closest scenario maybe would be when SCIP times out, there might still be useful information to be extracted in an observation. But that's not even obvious to me. There is no active node, so most of our observations are not even defined any more. And again, how would such an observation be used ? Are there RL algorithms which make use of a final observation ?

@gasse
Copy link
Member Author

gasse commented Jun 17, 2020

Maybe we can just keep things as they are now, and keep the issue open until we are sure what we want. There is no practical impact with this issue, just code refactoring really.

One argument for "officially" disabling final observations: then we can indicate that behaviour in the documentation, so that users are warned that they should not expect any obs in the final state.

@AntoinePrv AntoinePrv removed this from the v0.2.0 milestone Jun 30, 2020
@dchetelat dchetelat linked a pull request Jun 25, 2021 that will close this issue
@dchetelat
Copy link
Contributor

dchetelat commented Jun 29, 2021

There is no practical impact with this issue, just code refactoring really.

Ah! You jinxed it Maxime ;-) Turns out there was practical issues...

Fixed by #206.

@gasse
Copy link
Member Author

gasse commented Jun 29, 2021

Indeed :)

We should update the doc still, to mention this new behavior for final observations. I re-open the issue so that we don't forget about it.

@gasse gasse reopened this Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement 🚀 New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants