Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] Multilinear wrapper for econforge.interpolation #1151

Merged
merged 11 commits into from
Jul 8, 2022
Merged

[WIP] Multilinear wrapper for econforge.interpolation #1151

merged 11 commits into from
Jul 8, 2022

Conversation

Mv77
Copy link
Contributor

@Mv77 Mv77 commented Jul 5, 2022 •
edited
Loading

This PR adds a simple wrapper for multilinear interpolators from econforge.interpolation.

#1136 had some progress on this, but I believe the person in charge is no longer working on HARK. In addition, my simple class works for any dimension (linear, bilinear, trilinear); I've added tests that show how to use it and compare its output with HARK's current classes.

I was careful to also check for distance metrics, which might not seem important, but are what determines when to stop when solving an infinite horizon problem. My objects yield the same distance metrics as LinearInterp and BilinearInterp.

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

@Mv77
Copy link
Contributor Author

Mv77 commented Jul 5, 2022

I still need to write documentation for this class.

It only does basic interpolation/extrapolation. No fancy derivatives/limiting functions yet. However:

  • The econforge people say that ``derivatives are coming soon" In their docs.
  • I'm pretty sure they have derivatives already but they're just not documented. See this example. I might look into this at some point in the future.
  • I'm working on a downstream class with limiting functions/decay. That might take a while, because generalizing the decay machinery to n-dimensions is not straightforward (conceptually).

@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2022 •
edited
Loading

Codecov Report

Merging #1151 (be1b850) into master (d1cccc9) will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1151      +/-   ##
==========================================
+ Coverage   72.80%   72.99%   +0.19%     
==========================================
  Files          70       72       +2     
  Lines       11156    11237      +81     
==========================================
+ Hits         8122     8203      +81     
  Misses       3034     3034
Impacted Files Coverage Δ
HARK/econforgeinterp.py 100.00% <100.00%> (ø)
HARK/tests/test_econforgeinterp.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 d1cccc9...be1b850. Read the comment docs.

@Mv77
Copy link
Contributor Author

Mv77 commented Jul 6, 2022

@sbenthall I'd like to hear what you think of this and its relation to the existing PR. Do you know if the existing PR does something that this one does not? The code is kind of hard to follow.

@alanlujan91 I'd also like your feedback if possible. Specifically about:

  • You'll notice that the classes are not numba-fied. The reason is that I am under the impression that econforge.interpolation's method are already numba-fied. Do you know if this is correct? Is there any obvious speed-gain that could be added to what I have currently?
  • I know you also have your private wrapper class. Any suggestion on how this could be improved?

@llorracc
Copy link
Collaborator

llorracc commented Jul 7, 2022 via email

@sbenthall
Copy link
Contributor

@Mv77 I think PR #1136 is different in the following respects:

  • it modifies existing interpolators rather than creating a new class
  • it does some experimenting with the scipy interpolator

But it is incomplete, and it doesn't have the clear advantage of yours, which is the multidimensional interpolator.
I don't think #1136 should block this PR, which looks like great work to me.

@alanlujan91
Copy link
Member

I agree, this is great work and I'll go ahead and merge it!

@alanlujan91
Copy link
Member

@alanlujan91 I'd also like your feedback if possible. Specifically about:

  • You'll notice that the classes are not numba-fied. The reason is that I am under the impression that econforge.interpolation's method are already numba-fied. Do you know if this is correct? Is there any obvious speed-gain that could be added to what I have currently?
  • I know you also have your private wrapper class. Any suggestion on how this could be improved?
  1. does not need to be additionally numba-fied as econforge takes care of that, and there's not much to numba-fy on our side. Perhaps the exponential decay could use numba but that's for another PR.
  2. No comments on how to improve this. It's very close to what I was doing except I had retained the Linear, Bilinear, Trilinear framework. This takes care of all 3 and more.

@alanlujan91 alanlujan91 merged commit caaf9cc into econ-ark:master Jul 8, 2022
@Mv77 Mv77 mentioned this pull request Jul 9, 2022
Closed
@sbenthall sbenthall added this to the 0.13.0 milestone Jan 4, 2023
@sbenthall sbenthall mentioned this pull request Jan 4, 2023
Closed
3 tasks
@alanlujan91 alanlujan91 mentioned this pull request Feb 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Mv77 @codecov-commenter @llorracc @sbenthall @alanlujan91