Fix cons bequest

[alanlujan91]

@mnwhite I think I fixed what we talked about over email

@dedwar65 also I think this fixes the bug you were dealing with

I don't like the way I did this, with some repetitive code, but hopefully it gets it done.

@dedwar65 I can't add you as a reviewer, but please review this

[dedwar65]

I've just reviewed this. I think it should be merged in . The bug I was encountering should be resolved with this, but I will need to sync my fork with these newly merged in changes to see for sure.

Although, i'm not sure why some of the checks are failing.

[mnwhite]

Yes, this is the solver fix I mentioned. Let me look at the failing checks to see what's up.

[mnwhite]

It looks like a tiny discrepancy got introduced into the KinkedR model with this PR, possibly including an issue with the mNrm grid for its value function. I'll take a look.

[alanlujan91]

@mnwhite I think I know what's going on, I will push a change right away

[mnwhite]

Well that was easy to find. This PR swaps CubicHermiteInterp for CubicInterp in ConsIndShockModel, and I don't know what the new interpolator does / how it works. It looks like it has a check for x_list to be strictly increasing, which will cause problems when constructing EndOfPrd_vFunc in the KinkedR model: a=0 appears twice consecutively. That's easily fixable in one line.

The discrepancy in the cFunc of 0.0001 at the query/test point is due to the change in the cubic interpolator. I expected more consistent accuracy between cubic interpolator versions, so this might be a real error. Digging deeper...

[alanlujan91]

@mnwhite you are right, the fix i just made removes CubicHermite

There might be a true error somewhere else, but I think for the sake of this PR the change i made is sufficient, and we can address the remaining issue in a separate PR

[alanlujan91]

pre-commits are still failing, which are addressed in #1396, but hopefully everything else should pass

[mnwhite]

Let's see if the checks pass here, then merge. I agree we can do the other fix in another PR-- it's a pre-existing "harmless" error that was exposed by CubicHermite's additional check. It doesn't cause problems in the current tests because the only way the double a=0 would cause problems is if EndOfPrd_vFunc were evaluated at exactly zero (and even then it might not actually throw a NaN, it depends on how searchsorted works). It should be fixed, but it's not critical here.

But it looks like there might be a formatting "error", ugh.

[alanlujan91]

@mnwhite I propose we merge this if it passes everything else, then we merge #1396 which should fix the formatting error issue

[mnwhite]

Sounds good.

[codecov]

Codecov Report

Attention: Patch coverage is 6.06061% with 62 lines in your changes are missing coverage. Please review.

Project coverage is 71.54%. Comparing base (649d8c6) to head (2914149).
Report is 23 commits behind head on master.

Files	Patch %	Lines
HARK/ConsumptionSaving/ConsBequestModel.py	0.00%	62 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1392      +/-   ##
==========================================
- Coverage   71.69%   71.54%   -0.15%     
==========================================
  Files          84       84              
  Lines       13939    13944       +5     
==========================================
- Hits         9993     9976      -17     
- Misses       3946     3968      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mnwhite]

The only failing tests here are the formatting, which is being fixed in another PR, and codecov. But the only untested new lines are additional parameter typing checks, which would throw errors if we wrote tests for. I'm merging this, then will look at and merge the pre-commit PR.