lecture-python-advanced icon indicating copy to clipboard operation
lecture-python-advanced copied to clipboard

Refactorize code related with AMSS

Open shizejin opened this issue 4 years ago • 9 comments

We want to refactorize the code related with AMSS including a series of lectures including opt_tax_recur and amss (and potentially amss2 and amss3 in the near future) by a collaboration with @QBatista and @thomassargent30.

The parts to modify are mainly two Python classes

  1. Sequential Allocation problem
    • [ ] SequentialAllocation both in opt_tax_recur and amss
  2. Recursive Allocation problem
    • [ ] RecursiveAllocation in opt_tax_recur
    • [ ] RecursiveAllocationAMSS in amss

Our goal is to improve the quality of the code in several aspects:

  1. readability
    1. math notations of model parameters and variables
    2. make the code closely and clearly related with the computational algorithm described using math and avoid inconsistency
  2. efficiency
    1. optimize the root finding procedure

@QBatista would you please add on your thoughts if I missed anything? I remember that you wanted to jit the class. For me the performance of the current code is quite acceptable but it would be great to discuss if jitting would be needed.

We need to notice that these two lectures share a large fraction of the code so it would be great if we could avoid "copy and paste". We would appreciate it a lot if @mmcky has some suggestions on this issue.

I created a new branch mod_amss and we will modify the .rst files and make a PR from there.

shizejin avatar May 05 '21 10:05 shizejin

I refactorized SequentialAllocation first and here is the notebook of comparison.

Please see the detailed description inside.

shizejin avatar May 05 '21 10:05 shizejin

@shizejin you can bring code in from a file using the :load: option such as

```{code-cell}
:load: <path to file.py>
```

that is how we typically share executable code across lectures that needs to be visible on the site (i.e. doesn't come from a package like quantecon)

Note: I just realised this is only relevant syntax for https://github.com/QuantEcon/lecture-python-advanced.myst

mmcky avatar May 05 '21 23:05 mmcky

@shizejin this is looking like a pretty big change. I am thinking of starting the migration process to https://github.com/QuantEcon/lecture-python-advanced.myst.

Can you let me know your estimated timeframe on this change? Just wondering if I get you to submit a PR here or https://github.com/QuantEcon/lecture-python-advanced.myst. Thanks.

mmcky avatar May 06 '21 03:05 mmcky

Thanks @mmcky for your suggestion! It looks like the current syntax for the two lectures are

.. literalinclude:: <path to file.py>

but we were thinking about not making the duplicated code visible in every lecture. But I guess this may not be a very good idea since it harms the self-containedness of the lectures. Please let us know what you think.

And we estimate this to be accomplished at the end of May. Not sure when this will be done specifically within May though. When do you plan to start the migration process originally?

shizejin avatar May 06 '21 10:05 shizejin

Hi @shizejin. The current literalinclude is the equivalent of using:

```{code-cell}
:load: <path to file.pu>
```

in the new jupyter-book setup.

If the code isn't visible you can include it in a lecture but use tags to hide the code cell (if you want the output) or hide both the code-cell and the output (in the html) but they should be contained in download notebooks or any other execution environment.

mmcky avatar May 14 '21 04:05 mmcky

@shizejin I have been migrating the lectures over to jupyter-book but amss series seems to have an issue (perhaps with the current version of scipy?)

https://github.com/QuantEcon/lecture-python-advanced.myst/issues/44

mmcky avatar May 14 '21 04:05 mmcky

Hi @mmcky, I am about to make a PR about opt_tax_recur. May you please let me know whether I should submit a PR here or to https://github.com/QuantEcon/lecture-python-advanced.myst? Thanks!

One good news: opt_tax_recur had the same warning messages as you pointed out in the amss series, and they all disappeared after refactorization. I believe @QBatista 's refactorization would resolve the same issues in the amss series too.

shizejin avatar Jun 01 '21 02:06 shizejin

thanks @shizejin this is really exciting.

If you commit here I can transfer to lecture-python-advanced.myst using sphinx-tomyst. We are going through final checks for lecture-python-advanced.myst but not 100% sure when we will be able to make the final switch (maybe 1 more week). If you're comfortable with jupyter-book feel free to setup a PR https://github.com/QuantEcon/lecture-python-advanced.myst otherwise here is fine.

mmcky avatar Jun 01 '21 02:06 mmcky

@mmcky thanks! I think I will just make a PR here.

shizejin avatar Jun 01 '21 04:06 shizejin