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.

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

Gradients/derivatives in econforge interpolators #1157

Merged
merged 13 commits into from
Aug 19, 2022

Conversation

Mv77
Copy link
Contributor

@Mv77 Mv77 commented Jul 31, 2022

This PR adds the capability to compute first-order derivatives of n-dimensional interpolators for my wrapper of econforge.interpolation.py.

It works but:

  • We should only merge after Pablo has released the new version of econforge.interpolation.py. The current version one gets through pip has a bug. See Documentation for derivatives EconForge/interpolation.py#94. This PR depends on the version of econforge.interpolation currently in their master branch.
  • There are various improvements I'd like to make.

  • 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 added 2 commits July 29, 2022 08:58
Pablo says UC is for uniform grids and only worked previously due to a bug
@Mv77
Copy link
Contributor Author

Mv77 commented Jul 31, 2022

Improvements I'd like to make:

  • Add the option to evaluate derivatives w.r.t a single dimension (not the full gradient).
  • Add the option to simultaneously evaluate the function and its derivatives with the same function call. (to save time in figuring indices out).
  • Update requirements to use the version of interpolation.py that does not have the bug.

@llorracc
Copy link
Collaborator

llorracc commented Aug 1, 2022

This is great!

Though, failing some build tests...

@Mv77
Copy link
Contributor Author

Mv77 commented Aug 2, 2022

Yes!

The reason is that the current version of econforge/interpolation.py that HARK pulls has not fixed the bug! (I think we need to ask Pablo to make a release and require the new version when he does).

@llorracc
Copy link
Collaborator

llorracc commented Aug 2, 2022

Q: If you ask for the derivative exactly at a kink point (where the answer is in principle $\infty$), what does it return?

Or, are piecewise linear splines not allowed?

@Mv77
Copy link
Contributor Author

Mv77 commented Aug 3, 2022

Q: If you ask for the derivative exactly at a kink point (where the answer is in principle ∞), what does it return?

I just checked, and it calculates derivatives 'backwards' (using the previous point). Consider the following example

x = np.array([0.,1.,2.,3.,4.])
y = np.array([0.,0.,1.,3.,6.])

interp = LinearFast(y, [x])
grad = interp.gradient(x)
print(grad)

It creates a piecewise linear function. The derivative at 1 is (0 from the left) or (1 from the right). The derivative at 2 is (1 from the left) or (2 from the right). And so on. Here is what I get:

grad = array([0., 0., 1., 2., 3.])

So it's using left-differences.


Or, are piecewise linear splines not allowed?

Yes they are, and they are in fact the only thing I am using at the moment. I have not explored higher order splines.

@llorracc
Copy link
Collaborator

llorracc commented Aug 3, 2022

Hmmm, I guess that's better than $\infty$ or $-\infty$ for preventing the computer blowing up. But maybe it SHOULD blow up if you ask for a derivative at a point where it is undefined ...

@Mv77
Copy link
Contributor Author

Mv77 commented Aug 3, 2022

Hmmm, I guess that's better than ∞ or −∞ for preventing the computer blowing up. But maybe it SHOULD blow up if you ask for a derivative at a point where it is undefined ...

But we would not want to go around blowing up our---or other people's---computers!

@Mv77
Copy link
Contributor Author

Mv77 commented Aug 5, 2022

@MridulS , @sbenthall , this PR relies on the latest release of Pablo's interpolation.py, which is 2.2.2. There was a bug in previous versions, so if people get the wrong version they will be getting incorrect derivatives.

How do I reflect that in HARK's requirements? Is it a matter only of writing interpolation >= 2.2.2 in requirements.txt or should I do something else?

@MridulS
Copy link
Member

MridulS commented Aug 6, 2022

How do I reflect that in HARK's requirements? Is it a matter only of writing interpolation >= 2.2.2 in requirements.txt or should I do something else?

Yeah, that will make sure that HARK installed via github is depending on interpolation >= 2.2.2. But we need to make a new release to make sure if someone does pip install ... they get the proper version of interpolation.

@Mv77
Copy link
Contributor Author

Mv77 commented Aug 6, 2022

How do I reflect that in HARK's requirements? Is it a matter only of writing interpolation >= 2.2.2 in requirements.txt or should I do something else?

Yeah, that will make sure that HARK installed via github is depending on interpolation >= 2.2.2. But we need to make a new release to make sure if someone does pip install ... they get the proper version of interpolation.

Well the good thing is that none of the content currently in HARK depends on the buggy derivatives, those are only used by elements introduced in this PR. So even without a HARK release people won't be getting bugs from HARK. But when we do roll out these new features we have to enforce interpolation>=2.2.2

@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2022

Codecov Report

Merging #1157 (06ebf28) into master (4f950f0) will increase coverage by 0.24%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #1157      +/-   ##
==========================================
+ Coverage   73.33%   73.58%   +0.24%     
==========================================
  Files          72       72              
  Lines       11387    11465      +78     
==========================================
+ Hits         8351     8436      +85     
+ Misses       3036     3029       -7     
Impacted Files Coverage Δ
HARK/econforgeinterp.py 82.85% <72.72%> (-17.15%) ⬇️
HARK/interpolation.py 43.29% <100.00%> (+0.76%) ⬆️
HARK/tests/test_econforgeinterp.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Mv77
Copy link
Contributor Author

Mv77 commented Aug 6, 2022

Bleh. Current tests are failing in windows because interpolation.py requieres a package "curses" that is included in base Python for other OSs but not for Windows.

How on earth do I handle that requirement...

Edit: The requirement was accidental, Pablo has patched the issue.

@Mv77
Copy link
Contributor Author

Mv77 commented Aug 7, 2022

After Pablo's patch this is now ready to review.

@Mv77 Mv77 changed the title [WIP] Gradients/derivatives in econforge interpolators Gradients/derivatives in econforge interpolators Aug 7, 2022
@Mv77 Mv77 added Status: Review Needed Ready-To-Merge Has been reviewed and when branch is updated and checks pass it should be merged labels Aug 7, 2022
# Conflicts:
#	Documentation/CHANGELOG.md
#	HARK/econforgeinterp.py
@Mv77
Copy link
Contributor Author

Mv77 commented Aug 18, 2022

@llorracc @sbenthall @alanlujan91, could anyone review this? It's ready to go

Copy link
Member

@alanlujan91 alanlujan91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR, left a few comments. Looking forward to implementing this on other models.

HARK/tests/test_econforgeinterp.py Outdated Show resolved Hide resolved
HARK/tests/test_econforgeinterp.py Outdated Show resolved Hide resolved
HARK/tests/test_econforgeinterp.py Show resolved Hide resolved
@Mv77
Copy link
Contributor Author

Mv77 commented Aug 18, 2022

@alanlujan91 thanks for the great suggestions. The last couple of commits address them all. Don't merge this in yet, I think Chris wanted to chat about it first.

@Mv77
Copy link
Contributor Author

Mv77 commented Aug 19, 2022

@alanlujan91 approves and I've discussed @llorracc 's questions with him over email, so I'll go ahead and merge. Thanks everyone!

@Mv77 Mv77 merged commit a56c58c into econ-ark:master Aug 19, 2022
@sbenthall sbenthall added this to the 0.13.0 milestone Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-To-Merge Has been reviewed and when branch is updated and checks pass it should be merged Status: Review Needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants