ENH: Add state_values to DiscreteDP - #832
Conversation
|
@HG-Cheng Thank you for the contribution! Main comments
Other commentsPlease do not include changes in the files |
…e accidental files
|
"Hi @oyamad, thanks for the detailed review and guidance! I completely understand. I will remove action_values and num_actions to keep this PR focused on state_values only, and we can leave the state-action pair discussion for the future. I will pass state_values=state_values to MarkovChain in controlled_mc as suggested. Apologies for the accidental inclusion of .vscode/settings.json and _matrix_eqn.py! They leaked in from my local workspace and another task. I will remove them from this branch right away. I'll push the updated commits shortly!" |
# Conflicts: # quantecon/markov/ddp.py
|
Thanks again for the review. I've updated the PR to keep it focused on I also synced the branch with the latest |
oyamad
left a comment
There was a problem hiding this comment.
Thanks @HG-Cheng! One main request: please follow what is done in MarkovChain:
- Implement
state_valuesas a@propertywith a validating setter (storing toself._state_values), and have__init__delegate to it byself.state_values = state_values, as inMarkovChain.__init__. - You might also use
MarkovChain's error messages and setter docstring verbatim (e.g.,'state_values must be an array_like of length n').
|
Thanks for the review, @oyamad! I've updated
The focused tests and the full local test suite pass ( Could you please take another look when convenient? Thanks! |
This PR partially addresses #248 by adding
state_valuessupport toDiscreteDP.Changes:
state_valuesargument toDiscreteDP.state_valuesconsistently withMarkovChain.state_valueswhen converting between product andstate-action-pair formulations.
state_valuesto theMarkovChainreturned bycontrolled_mc.conversion.
action_valuesandnum_actionsare intentionally left out of this PR,following the review discussion.