implement CubicHermiteInterp

[alanlujan91]

CubicHermiteInterp is a HARK compatible wrapper of scipy's CubicHermiteSpline

See notebook for discussion: https://github.com/alanlujan91/HARK/blob/scipyCubic/examples/Interpolation/CubicInterp.ipynb

Please ensure your pull request adheres to the following guidelines:

• Tests for new functionality/models or Tests to reproduce the bug-fix in code.
• Updated documentation of features that add new functionality.
• Update CHANGELOG.md with major/minor changes.

[sbenthall]

This is great work @alanlujan91 !

I would be in favor of this replacing HARK's current cubic interpolator.

What happens if you do that? Do the automated tests for the library still pass?
In particular, do these unit tests for the interpolated pass?
https://github.com/econ-ark/HARK/blob/master/HARK/tests/test_interpolation.py#L46

(There are other model tests that use the interpolator.)

I wonder if it's possible to streamline the HARK interpolator interface further, but that may be out of scope of this particular PR.

[codecov-commenter]

Codecov Report

Merging #1011 (b51ea9e) into master (d59269d) will decrease coverage by 0.00%.
The diff coverage is 69.11%.

@@            Coverage Diff             @@
##           master    #1011      +/-   ##
==========================================
- Coverage   72.53%   72.53%   -0.01%     
==========================================
  Files          68       68              
  Lines       10298    10358      +60     
==========================================
+ Hits         7470     7513      +43     
- Misses       2828     2845      +17     

Impacted Files	Coverage Δ
HARK/interpolation.py	42.52% <68.18%> (+0.99%)	⬆️
HARK/ConsumptionSaving/ConsIndShockModel.py	85.86% <100.00%> (+0.01%)	⬆️
HARK/tests/test_interpolation.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d59269d...b51ea9e. Read the comment docs.

[llorracc]

@mnwhite

Could you review this per our conversation? Basically either ask Alan to add the old interpolator as an option (easier) or describe to him the tests you'd like him to conduct to show that the new version is better in every respect (including the memory usage issue that you identified earlier).

[mnwhite]

I'll try to take a look at this. The easiest/quickest way forward is to use the new version as CubicInterp and keep the old one with a marking of LEGACY or OLD. If it has the same interface as the old code, then everything should work the same. But then any project that needs the old version for memory reasons has the option to do that: `from HARK.interpolation import CubicInterpOLD` `CubicInterp = CubicInterpOLD`

…

On Mon, Jun 14, 2021 at 9:51 AM Christopher Llorracc Carroll < ***@***.***> wrote: @mnwhite <https://github.com/mnwhite> Could you review this per our conversation? Basically either ask Alan to add the old interpolator as an option (easier) or describe to him the tests you'd like him to conduct to show that the new version is better in every respect (including the memory usage issue that you identified earlier). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1011 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADKRAFK5HFHQYYZXMZS6W5DTSYCNBANCNFSM44HN6G6Q> .

[alanlujan91]

What happens if you do that? Do the automated tests for the library still pass?
In particular, do these unit tests for the interpolated pass?
https://github.com/econ-ark/HARK/blob/master/HARK/tests/test_interpolation.py#L46

@sbenthall I have swapped out CubicInterp for CubicHermiteInterp in ConsIndShockModel.py and in test_interpolation.py. Locally it passes all tests, waiting to see if it passes remotely.

[sbenthall]

Apologies for the slow review turnaround, @alanlujan91 .
I see there's a merge conflict on interpolation.py

Could you fix that merge conflict?

@mnwhite did you see any reason not to merge this PR?

[alanlujan91]

Could you fix that merge conflict?

@sbenthall please see #1060. I think this is something that should be addressed on its own in a standalone PR. I will work on this but lmk if you have any suggestions.

[sbenthall]

Ok, should this PR be closed because it has been superseded by #1060?