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

ENH: added routines to convert ddp between full and SA formulations #318

Merged
merged 3 commits into from
Aug 4, 2017

Conversation

sglyon
Copy link
Member

@sglyon sglyon commented Jul 31, 2017

Sometimes I find it helpful to have convenient, even if not optimally efficient, routines for converting between the SA and full formulations.

This PR implements such routines.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 92.168% when pulling 9938ff0 on sl/ddp_conversions into a82ba69 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 92.168% when pulling 9938ff0 on sl/ddp_conversions into a82ba69 on master.

if self._sa_pair:
ns = self.num_states
na = self.a_indices.max() + 1
R = np.zeros((ns, na))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be np.full((ns, na), -np.inf)?

if self._sa_pair:
return self
else:
s_ind, a_ind = np.where(np.isfinite(self.R))
Copy link
Member

Choose a reason for hiding this comment

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

Better to write self.R > -np.inf instead of np.isfinite(self.R)?
(self.R may contain np.inf.)

@oyamad
Copy link
Member

oyamad commented Aug 1, 2017

A comment on terminology:
I guess the terminologies should be consistent between the code and the documentation: "product formulation" should be replaced "full formulation" in the documentation, or vice versa.

(My view is that an SA formulation is already a "full" description of the model when the sets A(s) of available actions at state s are given excluding infeasible actions.)

@sglyon
Copy link
Member Author

sglyon commented Aug 1, 2017

Thanks for the review. I 100% agree on all accounts and will make the changes in a few hours when I'm back at a computer

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 92.168% when pulling 79a829c on sl/ddp_conversions into a82ba69 on master.

@oyamad
Copy link
Member

oyamad commented Aug 2, 2017

In the test code, maybe better to assign -np.inf to some entry in R and 0 in Q.
Otherwise the code looks good to me.

@sglyon
Copy link
Member Author

sglyon commented Aug 2, 2017

Good call @oyamad -- I've added these tests.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 92.188% when pulling 181dee0 on sl/ddp_conversions into a82ba69 on master.

@mmcky
Copy link
Contributor

mmcky commented Aug 3, 2017

Thanks @sglyon and @oyamad for review. This looks good to me. I will leave open one more day and then merge.

@mmcky mmcky added the ready label Aug 3, 2017
@mmcky
Copy link
Contributor

mmcky commented Aug 4, 2017

Thanks @sglyon.

@mmcky mmcky merged commit c1a5521 into master Aug 4, 2017
@mmcky mmcky deleted the sl/ddp_conversions branch August 4, 2017 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants