Skip to content

Random MarkovChain #154

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

Merged
merged 50 commits into from
Aug 7, 2015
Merged

Random MarkovChain #154

merged 50 commits into from
Aug 7, 2015

Conversation

oyamad
Copy link
Member

@oyamad oyamad commented May 20, 2015

(This is not yet ready to merge.)

I wrote some functions that generate random MarkovChain or stochastic matrix. I hope this module will be useful at least for developers and probably for users as well.

  1. random_probvec generates random probability vectors (whose entries are nonnegative and sum to one). (Note that just generating random numbers from [0, 1] and divide them by the sum would generate probability values that are skewed toward 0.5.)
  2. If one wants an n x n stochastic matrix with a small number of states with positive probabilities for each row, where the number k of such states must be common across states here, then k integers have to be chosen randomly from 0, 1, ..., n-1, which is done by random_choice_without_replacement.
  3. random_markov_chain will support sparse matrix format once MarkovChain does.
  4. random_choice_without_replacement(n, k, num_trials=None) is equivalent to np.random.choice(n, k, replace=False) and is much (100x?) faster with Numba.
  5. random_choice_without_replacement may be included in utilities.py. Its name seems too long. Anybody have any idea?
  6. API regarding random_state will be modified once we fix a common interface (I opened a new issue Option to supply a random seed #153 to discuss this).

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.44%) to 88.61% when pulling 4269721 on random_mc into 2b3fac1 on master.

@mmcky
Copy link
Contributor

mmcky commented May 22, 2015

@oyamad Neat - It will be good to have this for easily creating some matrix for testing or demonstration etc.

Regarding the name. What do you think about just mimicking the np structure by adding a random subpackage or module? We could then use the hierarchy of the package to capture the essence contained in the name random_choice_without_replacement as is done in numpy.

import quantecon as qe
qe.random.choice(n, k, replace=False)

@oyamad
Copy link
Member Author

oyamad commented May 22, 2015

@mmcky Thank you for your comments. It is a good idea to make it parallel to NumPy, which would make the names easier to remember.

Just for information:

  • The same functionality in the standard library is called random.sample. (In fact I took its code and jitted it with an additional outer loop which is for the case where num_trials is not None.)
  • I also found scikit-learn has the same functionality which is named random.sample_without_replacement [source].

@mmcky
Copy link
Contributor

mmcky commented May 24, 2015

@oyamad Thanks for the additional info - good to know. Given how much numpy is used in this package - I think numpy probably seems to make the most sense to mimic. Having said that I like the scikit-learn use of sample rather than choice but not really a big deal.

Happy to help re-package this as a subpackage if you like.

@oyamad
Copy link
Member Author

oyamad commented May 26, 2015

@mmcky Please feel free to edit the branch, thanks!

Maybe we would better fix the interface for random_state first.

@oyamad
Copy link
Member Author

oyamad commented May 29, 2015

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.38%) to 88.67% when pulling d71c006 on random_mc into 2b3fac1 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.38%) to 88.67% when pulling 159076d on random_mc into 2b3fac1 on master.

@oyamad
Copy link
Member Author

oyamad commented Jun 7, 2015

I think this PR is ready for merging. Importantly, this contains the feature of random_state option with util.check_random_state, so that after this we can start working on adding the option to other functions/classes (unless anyone has different thoughts).

(Sparse matrix support and renaming and moving random_choice_without_replacement can be postponed.)

Add the same test case as in the Julia version
@mmcky
Copy link
Contributor

mmcky commented Aug 3, 2015

Hi @oyamad - Sorry for my delay. Just reacquainting myself with this issue. This is looking good. Are you still happy for this to be merged? I see that nosetests is reporting all OK for test_random_mc.py.

@mmcky
Copy link
Contributor

mmcky commented Aug 3, 2015

Does this need to be updated prior to merge?

  • random_choice_without_replacement: rename and move to random subpackage or module

@mmcky
Copy link
Contributor

mmcky commented Aug 3, 2015

@oyamad I am happy to move code around to fulfil the checkbox above.
Let's hold off on sparse matrix support and open a separate issue so this can be merged (as you suggest).

I wanted to run a few things by you before I implement this:

  1. I have previously indicated that we should probably emulate numpy given its strong use throughout the package. However I think sample is more aligned with common terminology rather than the numpy use of choice. Would you mind if we changed to sample?
  2. I see a number of useful utilities in random_mc. I propose to move random_choice_without_replacement to sample(replace=False) where replacement is an option to a sample function that resides in util/random.py. We can then fix up the API to call qe.random.sample(replace=False) for random_choice_without_replacement
  3. Should we also move random_probvec to util/random.py with usage qe.random.probvec()
  4. Should we also move random_stochastic_matrix to util/random.py with usage qe.random.stochastic_matrix()?

Thanks

@oyamad
Copy link
Member Author

oyamad commented Aug 4, 2015

@mmcky

1, 2: Please do go ahead as you suggest.

3, 4: @jstac once mentioned having a subpackage (say markov) that collects routines regarding markov chains and markov decision processes. random_probvec and random_stochastic_matrix might be put there as qe.markov.random.probvec (or qe.markov.random.probability_vector) and qe.markov.random.stochastic_matrix. (I have no strong opinion though.)

@jstac
Copy link
Contributor

jstac commented Aug 4, 2015

I support having a markov subpackage.

I feel that names like qe.markov.random.probvec etc are too verbose. I propose that we put this function in somewhere like util and perhaps make it visible in the top level qe namespace. I'm not sure about random_stochastic_matrix. Either subpackage would be fine.

@mmcky
Copy link
Contributor

mmcky commented Aug 4, 2015

@oyamad I only came across one instance of mc_tools in lucas_stokey.py. This has been updated and will now run.

@mmcky
Copy link
Contributor

mmcky commented Aug 4, 2015

@oyamad I think the reorganisation is complete. When you have some time - it would be great if you could have a quick look at the branch. Once that is done we should go ahead and get approval to merge.

@oyamad
Copy link
Member Author

oyamad commented Aug 5, 2015

@mmcky Many thanks!

I will have a closer look tomorrow, but one quick comment: maybe gth_solve.py also should be put in markov.

@mmcky
Copy link
Contributor

mmcky commented Aug 5, 2015

@oyamad Great thanks. I have moved gth_solve to the markov subpackage and updated relevant __init__.py files and the associated test suite.

@oyamad
Copy link
Member Author

oyamad commented Aug 6, 2015

@mmcky Thanks!

It seems that MarkovChain etc can be called by both qe.MarkovChain and qe.markov.MarkovChain. Which do we recommend the user to use? (Functions/classes in models can only be called with qe.models.*.)

oyamad added 3 commits August 6, 2015 15:34
instead of random_probvec, random_sample_without_replacement from ..util
move test_sample_without_replacement_* from markov/tests to util/tests
@oyamad
Copy link
Member Author

oyamad commented Aug 6, 2015

I made a few changes:

  1. Examples sections updated (subject to change after we fix a style guide; see Improve online documentation #139).
  2. I replaced random_probvec and random_sample_without_replacement with probvec and sample_without_replacement; I think at least in this case there is no need to give two names to one function. (I didn't touch the __init__.py files.)
  3. I moved test_sample_without_replacement_* from markov to util/tests/test_random.py.
    (There, from quantecon.random import sample_without_replacement did not work, so I imported with from quantecon.util.random import sample_without_replacement. Is this correct?)

…ing relevant utility functions from the utilities subpackage which is geared more towards internal utilities and package infrastructure
@mmcky
Copy link
Contributor

mmcky commented Aug 6, 2015

@oyamad These are excellent points. I have discussed with @jstac today and we have decided to do a bit more reorganisation. The util subpackage is probably best geared towards internal quantecon utilities that support the package infrastructure. This includes functions like the check_random_seed and special testing functions etc. Therefore I have moved probvec and sample_without_replacement to a random subpackage. These are topical functions that we can use via the package structure and should be accessed by

qe.random.probvec()
qe.random.sample_without_replacement()

Thanks for updating the examples section, I had only focused on the examples/ folder and neglected the docstrings. I will check those docstrings now based on this new round of revisions.

I have moved associated tests

@oyamad
Copy link
Member Author

oyamad commented Aug 6, 2015

@mmcky Very good, thanks. I am now very happy with this PR.

This is an independent issue probably for another PR: some other files might also be moved to util, such as

  • common_messages.py
  • external.py
  • timing.py

@mmcky
Copy link
Contributor

mmcky commented Aug 6, 2015

Great thanks for the feedback @oyamad. Good idea re: migration to util will add to a separate issue.
I have also migrated tauchen.py to the markov package

@cc7768
Copy link
Member

cc7768 commented Aug 6, 2015

I have also migrated tauchen.py to the markov package

What are the chances we can rename the tauchen.py file to something along the lines markov_approximation.py and then name the approx_markov function tauchen? This has always made more sense to me, and I think we've talked about it, but I don't remember what discussion has been had.

If we do this, then I am happy to rewrite (or find my code) to do the Rouwenhorst approximation as well and can drop it in there.

@mmcky
Copy link
Contributor

mmcky commented Aug 6, 2015

@cc7768 Good idea. I will do this revision as part of this PR. Let's add Rouwenhorst approximation in another PR.

Having a look at QuantEcon.jl this will also harmonise the function names used.
@spencerlyon2 @cc7768 Given this rework putting markov items into a python markov subpackage - can we talk sometime about the structure of Julia packages. Should we have a markov subpackage in Julia also?

@mmcky
Copy link
Contributor

mmcky commented Aug 7, 2015

Merging this issue now. Thanks @oyamad for this addition. Thanks everyone for comments and suggestions.

mmcky added a commit that referenced this pull request Aug 7, 2015
Random MarkovChain and some Reorganisation of Repository into ``util``, ``markov`` and ``random`` sub packages.
@mmcky mmcky merged commit e1b6f48 into master Aug 7, 2015
@oyamad oyamad deleted the random_mc branch August 8, 2015 02:31
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.

6 participants