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

rewrite calcExpectation to use numpy methods, see #625 #897

Merged
merged 2 commits into from
Jan 8, 2021

Conversation

sbenthall
Copy link
Contributor

@sbenthall sbenthall commented Dec 27, 2020

This rewrites calcExpectation to use numpy.apply_along_axis.
It is a much more succinct implementation.

This rewritten version does not compute the tensor product in cases of multiple 1D value inputs.
I have argued in #625 that this should be handled by a different method outside of the calcExpectation function.

What this PR lacks in terms of functionality compared to the first implementation, it makes up for in automated tests.
This PR includes tests, including for multivariate distributions with both array and constant non-stochastic values.

  • 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
Copy link
Contributor Author

@Mv77 please review!

@Mv77
Copy link
Contributor

Mv77 commented Jan 7, 2021

This looks good and useful to me.

I am relying on the tests that you provide for my review for now, and they look awesome.

A more complete test would be to use this functionality in the solution of an actual model. I can try it in #832 as you have requested when I get back to working on it (I'll have to update it soon with all the cool stuff that's been added). I might have more useful things to say then.

But since this does not break anything and is a step in the right direction to potentially be improved upon in the future, I would say it passes review.

@sbenthall
Copy link
Contributor Author

Thanks for this review, @Mv77
I'll merge this now.

As you try to work calcExpectations in to #832 , keep an eye out for this kind of tensor product manipulation:
https://github.com/econ-ark/HARK/pull/897/files#diff-29545ab437dbe5ab93d947afb0a70205323f0f9eba1d565d965adb5ceed1c732L1052-L1071

It's likely we will should be supporting this sort of operation in a reusable way, but that there's a simpler implementation of it that leverages numpy better. See discussion in #625

@sbenthall sbenthall merged commit 98da4de into econ-ark:master Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants