lecture-julia.myst icon indicating copy to clipboard operation
lecture-julia.myst copied to clipboard

Improvements to optimal growth lecture

Open jstac opened this issue 3 years ago • 3 comments

There are some improvements that could be made to the lecture https://julia.quantecon.org/dynamic_programming/optgrowth.html

  • operator T is not type stable
  • using Optim can be dropped in favor of built in findmax
  • an OptimalGrowthModel struct could be introduced, while spells out the primitives (I find this helpful, to further clarify what the primitives are for a savings problem --- in this case u, f, beta, psi)

I probably wrote the first round of this code and I didn't do a great job. Happy to implement these changes @jlperla , if you agree.

jstac avatar Dec 31 '21 21:12 jstac

Thanks John. I will take a look. Sorry, schools postponed in Vancouver a week so won't be able to look for a bit.

The only thing I am vehemently against us using a struct. Misuse is the biggest source of hidden performance and over typing errors in Julia and my hope was to get rid of all of them (except where they need to be mutable) in the code.

The stuff with Optim we want to wait for the maximize sense in galacticoptim. I think there is already an issue for it

jlperla avatar Dec 31 '21 21:12 jlperla

The only thing I am vehemently against us using a struct. Misuse is the biggest source of hidden performance and over typing errors in Julia and my hope was to get rid of all of them (except where they need to be mutable) in the code.

I think it's more important to bring the code closer to the maths than worry about performance, in this case. Using a struct helps us emphasize that an optimal growth model is a tuple (u, f, beta, phi), where these objects have specified types.

The stuff with Optim we want to wait for the maximize sense in galacticoptim. I think there is already an issue for it

That's unnecessary for this very simple case. There are functions in Base that do the job just fine.

No rush to look at this. I'll put together a PR at some point.

jstac avatar Dec 31 '21 21:12 jstac

Oh I see that this is using a set of arguments rather than a named tuple. 100% agree that it is better to have named tuples for parameters than individual ones, and closer to the math. It is only the use of structs for simple things that I am opposed to. They are too easy to misuse for intro users...They basically need to understand the details of https://julia.quantecon.org/getting_started_julia/introduction_to_types.html#declaring-parametric-types-advanced Or else they can break autodiff unnecessarily and have silent orders of magnitude drops in performance with subtle mistakes. By the time they are ready for real projects they have figured it out though.

For findmax, I don't see how that is a replacement here since this actually runs a full continuous optimizer rather than just sequentially going through a discrete grid of all values and picking the largest? But maybe I will

The type stability issue definetely should be fixed, thanks for pointing this out. I think I was lazy on some of these.

This is a great one to discuss a few holistic changes that probably should be made across the board. Will try to write up stuff to discuss soon.

Thanks!

jlperla avatar Dec 31 '21 22:12 jlperla