Skip to content
This repository has been archived by the owner on Apr 26, 2023. It is now read-only.

added info interface, updated simulators to use it (sisl/DeepRL.jl#5) #43

Merged
merged 7 commits into from Jan 12, 2018

Conversation

zsunberg
Copy link
Member

Here's how I think the information interface should look.

There are four functions in src/model/info.jl. Problem writers and solver writers can implement them if they want to provide more information.

I also updated HistoryRecorder and stepthrough to use these functions.

I think action_info will be really useful for my online tree-based solvers because it will make it easier to save the trees for later examination.

Any thoughts @rejuvyesh and @MaximeBouton ?

@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 77.273% when pulling df62b44 on info into dfb351c on master.

Copy link
Contributor

@MaximeBouton MaximeBouton left a comment

Choose a reason for hiding this comment

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

I think the info interface is consistent with the rest of the package. I will try to use it on sample problems and give some user feedback.

One concern is should it stay in POMDPToolbox or go in POMDPs ?
The definition of generate_sr and generate_sor are in POMDPs.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 77.378% when pulling 74d11e8 on info into d776c60 on master.

@zsunberg
Copy link
Member Author

@MaximeBouton the reason to put it in POMDPToolbox is because it is peripheral and might be a distraction in the documentation of the core interface. I've generally been more happy putting things in POMDPToolbox than in POMDPs. If it becomes really important in the future, we can move it to POMDPs.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 77.378% when pulling eb6ff8d on info into d776c60 on master.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling bcdb0e8 on info into ** on master**.

@zsunberg
Copy link
Member Author

Hey @rejuvyesh , could you take a quick look at this and give it a thumbs up if you think it looks good? I'm about to merge it, but want to catch any problems you might see. Thanks!!

Copy link
Member

@rejuvyesh rejuvyesh left a comment

Choose a reason for hiding this comment

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

LGTM! Not sure we need to specify it's a Dict although it looks like it's only a suggestion.

@zsunberg zsunberg merged commit ba9804a into master Jan 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants