-
-
Notifications
You must be signed in to change notification settings - Fork 199
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
Simplify expectation taking code #625
Comments
duplicate #621 |
|
This code is calculating the expectation of:
|
Try to mirror the Sympy syntax: https://docs.sympy.org/latest/modules/stats.html?highlight=expectation#sympy.stats.Expectation |
I'm going to take a stab at this tonight or tomorrow morning. It's easier
than I thought.
…On Thu, Jun 18, 2020 at 1:06 PM Sebastian Benthall ***@***.***> wrote:
https://gist.github.com/mnwhite/a77387cf2633f0a2404342d39fdb5284
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#625 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFPCBZHMOC5NUIQLNQDRXJCPZANCNFSM4MJX676A>
.
|
By "this" you mean working out sympy syntax for some expression(s) in your gist code? |
I'm making a non-sympy expectations function. Here's the docstring, which won't format correctly here: `
` |
See the calcExpectations branch for a completely untested draft of the
function. It's what I was able to write in an hour before needing to jump
on baby duty. Will test it on nonsense functions to make sure it works,
then will pastebin an edited version of the ToddlerCSmodel.py thingy where
it's implemented.
…On Thu, Jun 18, 2020 at 5:07 PM Christopher Llorracc Carroll < ***@***.***> wrote:
By "this" you mean working out sympy syntax for some expression(s) in your
gist code?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#625 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFOT4HIAKIWXAEMER3LRXJ6ZHANCNFSM4MJX676A>
.
|
Right on. I'll be glad to review this PR when it's ready. In my view, it makes sense to make the problem easier by breaking it down. Refactoring the solution code into methods/functions/objections that create an abstraction layer of mathematical operations over the lower level implementation (which for now will be matrix manipulations but later may be symbolic) is very difficult for me because I don't have the math in my head. It makes tons of sense for @mnwhite to do that, if he has time. Massaging these functions into a smooth, Mathematica/Sympy inspired syntax is much easier (for me) once that first step is completed. My only quibble with what Matt's proposed is a style issue that it pervasive across the whole library and perhaps can't be helped. But I should bring it up nonetheless. According to PEP 8, the recommendation is:
I realize that this is a sticky problem but I'd volunteer to do the mass conversion before the 1.0 release. |
The capitalization style is intentional; to my recollection, Nate Palmer
told us this was the standard for functions when we began the project. The
lack of underscores in variable names has a different origin.
…On Fri, Jun 19, 2020 at 10:00 AM Sebastian Benthall < ***@***.***> wrote:
Right on. I'll be glad to review this PR when it's ready.
In my view, it makes sense to make the problem easier by breaking it down.
Refactoring the solution code into methods/functions/objections that
create an abstraction layer of mathematical operations over the lower level
implementation (which for now will be matrix manipulations but later may be
symbolic) is very difficult *for me* because I don't have the math in my
head. It makes tons of sense for @mnwhite <https://github.com/mnwhite> to
do that, if he has time.
Massaging these functions into a smooth, Mathematica/Sympy inspired syntax
is much easier (for me) once that first step is completed.
My only quibble with what Matt's proposed is a style issue that it
pervasive across the whole library and perhaps can't be helped. But I
should bring it up nonetheless. According to PEP 8
<https://www.python.org/dev/peps/pep-0008/>, the recommendation is:
- Class names should normally use the CapWords convention.
- Function names should be lowercase, with words separated by
underscores as necessary to improve readability. Variable names follow the
same convention as function names. So lower_case_with_underscores
mixedCase is allowed for "backwards compatibility", which was a nice
allowance given the variety of styles used in Python. But PEP 8 was
published in 2001. So it's a little silly for a project launched almost 15
years later to be using the deprecated convention.
I realize that this is a sticky problem but I'd volunteer to do the mass
conversion before the 1.0 release.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#625 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFK22MJDWI2XHNAQIL3RXNVQFANCNFSM4MJX676A>
.
|
@llorracc today in the meeting has pointed out that it would be nice if the object representing a random variable (for example, the distribution objects) were used both in simulation (when sampling) and in solution (by taking expectations). |
I think the current implementation of https://numpy.org/doc/stable/reference/generated/numpy.apply_along_axis.html |
One aspect of the This introduces a lot of complexity as the current method is trying to cover a couple different kinds of broadcasting without relying on the underlying numpy machinery. One small issue resulting from this is that the documentation is ambiguous as to whether the 'default' value of these https://github.com/econ-ark/HARK/blob/master/HARK/distribution.py#L1020-L1032 Correspondingly, I've found a small inconvenience coming from whether the output of Consider:
In this code https://numpy.org/doc/stable/reference/generated/numpy.dot.html I think it will save the user a lot of headache if we think through how to implement distributions and expectation taking in terms of numpy array broadcasting. |
The current The condition for using this tensor-product mechanism, as opposed to the alternative simpler case where the function simply broadcasts over the values array, is unclear. I don't think the current code/documentation successfully distinguished between https://github.com/econ-ark/HARK/blob/master/HARK/distribution.py#L1047-L1048 Instead of combining these two complex operations into a single function, it would be better to break them down into simpler steps. Creating the tensor product of several grids can be its own function and live elsewhere. That way the expectation-taking code can be focused on doing one thing well. I'll make this change in a PR. |
I'm not sure, but I believe that the intention of the grid 'tensor product' work can be done with this built-in numpy function for the Kronecker product: https://numpy.org/doc/stable/reference/generated/numpy.kron.html#numpy.kron Or possibly it can be done using this https://numpy.org/doc/stable/reference/generated/numpy.tensordot.html?highlight=tensor%20product Because this is a very simple operation, and one that could easily be done prior to the input to As |
Ok, I've tested it and https://numpy.org/doc/stable/reference/generated/numpy.apply_along_axis.html#numpy.apply_along_axis That makes the implementation of In general, HARK should be refactored to use the core libraries effectively as much as possible. |
It's actually quite wonderful. |
rewrite calcExpectation to use numpy methods, see #625
@sbenthall had mentioned he'd be keen to hear feedback on the new I have been incorporating it into my new portfolio model today. It works great: intuitive to use, surprisingly faster, and makes for clearer code, pushing HARK towards explicit definitions of transition equations. |
oh fantastic. thanks! |
Maybe the right numpy tool for making the grids over several scalar function inputs is np.meshgrid https://numpy.org/doc/stable/reference/generated/numpy.meshgrid.html |
I think that's it but I'm not sure what it would be needed for in the current implementation given that the multivariate distribution already has that grid for shocks. Maybe it could be used for |
Still, I believe there are various places in HARK where multiple lines of |
There should be a separate issue for this.
…On Wed, Jan 27, 2021, 11:48 AM Mateo Velásquez-Giraldo < ***@***.***> wrote:
Still, I believe there are various places in HARK where multiple lines of
np.tile(...) could be replaced with single lines of np.meshgrid.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#625 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAQZEEUSNHEFLEBW64SBXLS4A7U3ANCNFSM4MJX676A>
.
|
Closing this issue. The new |
The code that gets the expected values of distributions in the solver can be simplified by making generic methods on the distribution classes.
https://github.com/econ-ark/HARK/blob/master/HARK/ConsumptionSaving/ConsAggShockModel.py#L633-L667
The text was updated successfully, but these errors were encountered: