Skip to content

Refactorize code related with AMSS #203

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

Open
3 tasks
shizejin opened this issue May 5, 2021 · 9 comments
Open
3 tasks

Refactorize code related with AMSS #203

shizejin opened this issue May 5, 2021 · 9 comments

Comments

@shizejin
Copy link
Member

shizejin commented May 5, 2021

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
Copy link
Member Author

shizejin commented May 5, 2021

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

Please see the detailed description inside.

@mmcky
Copy link
Contributor

mmcky commented May 5, 2021

@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
Copy link
Contributor

mmcky commented May 6, 2021

@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.

@shizejin
Copy link
Member Author

shizejin commented May 6, 2021

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?

@mmcky
Copy link
Contributor

mmcky commented May 14, 2021

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
Copy link
Contributor

mmcky commented May 14, 2021

@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?)

QuantEcon/lecture-python-advanced.myst#44

@shizejin
Copy link
Member Author

shizejin commented Jun 1, 2021

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.

@mmcky
Copy link
Contributor

mmcky commented Jun 1, 2021

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.

@shizejin
Copy link
Member Author

shizejin commented Jun 1, 2021

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

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

No branches or pull requests

2 participants