jstac / jitting_py_lectures

3 stars 0 forks source link

[WIP] Jit ifp #8

Open shizejin opened 5 years ago

shizejin commented 5 years ago

The code has been jitted here. I am not sure about the following changes that I made:

  1. Originally c is used as consumption in Bellman Operator, but as policy array in the Coleman Operator. I use it as consumption in both operators to make it consistent, and use C as policy array.

  2. In this lecture, there are lots of comparison between Bellman operation and Coleman operation. So I add C_guess1 and C_guess2 as attributes to the ConsumerProblem to store the two different policy. But somehow these seem to be redundant. I am also happy to delete this if all of us find it unnecessary.

And there is one "problem":

I appreciate any help or suggestions. @jstac @cc7768 @QBatista @natashawatkins @bkaplowitz @rebekahanne

jstac commented 5 years ago

Thanks @shizejin, I appreciate it.

I'll look at this carefully when I have a bit of time.

natashawatkins commented 5 years ago

Hi @shizejin - thanks! I didn't think to use njit like that in the keyword arguments, nice work...

Couple of points:

I've somehow introduced an issue with the bellman operator which I will work to fix..

Some possible things to change:

Thanks again

natashawatkins commented 5 years ago

Fixed the bug

natashawatkins commented 5 years ago

You also probably can't jit the last exercise actually, because it contains the outer loop for solving the model

shizejin commented 5 years ago

Hi @natashawatkins , thanks a lot for double checking and the helpful suggestions.

jstac commented 5 years ago

@shizejin @natashawatkins Thanks guys, very nice work.

I've been a bit inconsistent with notation but let's use σ for the policy that the operator K acts on and let's also use the name K for the function created inside the function factory. We could go either way but perhaps let's keep it closer to the maths.

I we don't see a new version of interpolations.py in a week or two let's request one.

shizejin commented 5 years ago

Thanks @jstac , I have changed bellman_operator and coleman_operator to T and K.

Shall we change the policy function in the math to σ as well, or keep using c?

jstac commented 5 years ago

It would be good to change it in the maths as well. Would you mind to make that change?

shizejin commented 5 years ago

Sure. I will go through the notebook to make sure the math is consistent with the code.

shizejin commented 5 years ago

Things to do before merge:

Also @natashawatkins , do you mind to explain your idea about solve_model to me? I didn't quite understand it. Thanks!

natashawatkins commented 5 years ago

The lecture currently uses qe.compute_fixed_point to find the solution, but we've decided to actually write a function in the lecture called solve_model to find the fixed point - see optgrowth.ipynb for an example

natashawatkins commented 5 years ago

Also @jstac and I decided to cut most of the the discussion about timing and instead focus on other advantages of the Coleman operator - don't worry about this though. Once those last things are cleaned up (and interpolation.py is updated), I'll start editing the actual lecture

shizejin commented 5 years ago

Sure. Will add solve_model to the lecture probably tonight.

shizejin commented 5 years ago

solve_model is added to the solution of Exercise 2 and is used by Exercise 3 and 4 as well. The results are slightly different because solve_model is using a smaller error tolerance and larger maximum of iterations (the same with the one in optgrowth) comparing to qe.conpute_fix_point. I also delete verbose and print_skip because in this notebook usually there are comparison between policy with different parameters, and printing out the convergence info for each fixed point seems to be messy.

One thing confuses me that solving a fixed point should be introduced after Exercise 1, but in the hints of Exercise 3 there is already codes using qe.compute_fix_point. I guess we can either

  1. introduce solve_model before the hints of Exercise 3. But then the Exercise 1 would be kind of awkward.

  2. remove the code in hints of Exercise 3.

natashawatkins commented 5 years ago

@shizejin I think we should probably solve the model once in the lecture

Would you be able to remove the guess parameters from the class? Sorry!

shizejin commented 5 years ago

@natashawatkins

Sorry in advance if I misunderstood you.

natashawatkins commented 5 years ago

@shizejin I think we may actually get rid of that exercise because we're planning to remove the speed comparisons between the operators anyway

shizejin commented 5 years ago

@natashawatkins That sounds good to me.

How about the guess parameters? Should I replace them with some arrays? Just want to make sure what my task is. Thanks!

natashawatkins commented 5 years ago

No please remove them from the class, and just create an array for the questions that require calling T(v). Also just create the initial condition in solve_model

shizejin commented 5 years ago

@natashawatkins

natashawatkins commented 5 years ago

Great - thanks for your help @shizejin! This looks good

natashawatkins commented 5 years ago

Hi @shizejin I've updated the rst file in our build system here - https://github.com/QuantEcon/lecture-source-py/pull/94

It would be helpful if you could build the lecture (see instructions in README) and check that it's correct - I essentially had to copy-paste

shizejin commented 5 years ago

Hi @natashawatkins , I tried to build and there are two very minor issues:

  1. This is actually my fault. I left some meaningless empty lines in the docstrings, and I am afraid this could confuse the readers about the coding style. I've deleted them in the newest commitment, would you mind modifying the corresponding part in rst file? Sorry for the extra work!

  2. I guess you are using the old illustration for Exercise 4, but we have a slightly different output now as the tolerance for convergence of solve_model is different with qe.compute_fixed_point which was used. We'd better to update the figure.

All the rest look good to me! Thanks!

natashawatkins commented 5 years ago

Hi @shizejin, sorry to be a pain, but could you make the changes in the PR here (https://github.com/QuantEcon/lecture-source-py/pull/94). Let me know if you have any difficulties

shizejin commented 5 years ago

Hi @natashawatkins Sure, but could you give me the permission? I tried to commit but get denied.

natashawatkins commented 5 years ago

Ah sorry about that! @jstac would you be able to add @shizejin to the lecture-source-py repo

shizejin commented 5 years ago

@natashawatkins And actually it is just deleting 4 empty lines (line 411, 461, 514, 740 in the ifp.rst file). I guess it will take less than 1 min to do this. If you would like to help me modify, then it won't bother to add me to the repo :)

natashawatkins commented 5 years ago

I think we may as well add you to the repo at this point anyway :)