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

Fix "extra dimension" in distribution.calc_expectations #1149

Merged
merged 5 commits into from
Jun 23, 2022

Conversation

Mv77
Copy link
Contributor

@Mv77 Mv77 commented Jun 3, 2022

Fixes #1098

  • 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 Jun 3, 2022

I've fixed the function for now. I'm working on updating the many-many calls that took this issue as given.

@Mv77 Mv77 changed the title [WIP] Fix "extra dimension" in distribution.calc_expectations Fix "extra dimension" in distribution.calc_expectations Jun 3, 2022
@Mv77
Copy link
Contributor Author

Mv77 commented Jun 3, 2022

This is ready to merge. (Tests will pass, I just updated the changelog).

The one thing that becomes more ugly as a result of this PR is that calc_expectation(X) where X is a univariate distribution will return a size-1 array, not a scalar. This is the cost of making everything based around arrays and not having if-else statements figuring out the types of multiple possible inputs. It is a cost I'd gladly pay. That is why you will see, in places like check_conditions, things like calc_expectations(PermShkDstn)[0].

@codecov-commenter
Copy link

Codecov Report

Merging #1149 (4c7e780) into master (2155175) will decrease coverage by 0.01%.
The diff coverage is 96.05%.

@@            Coverage Diff             @@
##           master    #1149      +/-   ##
==========================================
- Coverage   74.07%   74.06%   -0.02%     
==========================================
  Files          70       70              
  Lines       10809    10807       -2     
==========================================
- Hits         8007     8004       -3     
- Misses       2802     2803       +1     
Impacted Files Coverage Δ
HARK/ConsumptionSaving/ConsGenIncProcessModel.py 82.45% <ø> (+0.13%) ⬆️
HARK/ConsumptionSaving/ConsPortfolioModel.py 88.69% <ø> (-0.22%) ⬇️
...RK/ConsumptionSaving/tests/test_ConsMarkovModel.py 86.56% <ø> (ø)
...K/ConsumptionSaving/tests/test_modelcomparisons.py 98.46% <ø> (ø)
HARK/distribution.py 84.41% <90.32%> (-0.98%) ⬇️
HARK/ConsumptionSaving/ConsIndShockModel.py 85.86% <100.00%> (ø)
HARK/ConsumptionSaving/ConsLaborModel.py 82.90% <100.00%> (ø)
HARK/ConsumptionSaving/ConsMedModel.py 74.04% <100.00%> (ø)
HARK/ConsumptionSaving/ConsPrefShockModel.py 79.68% <100.00%> (ø)
HARK/ConsumptionSaving/ConsRiskyContribModel.py 79.42% <100.00%> (ø)
... and 5 more

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 704bf8e...4c7e780. Read the comment docs.

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.

Looks great!

@Mv77
Copy link
Contributor Author

Mv77 commented Jun 23, 2022

@llorracc @sbenthall I'm merging this in given that it is minor and passed tests and Alan's review.

Thanks @alanlujan91 !

@Mv77 Mv77 merged commit 0171e0d into econ-ark:master Jun 23, 2022
@Mv77 Mv77 deleted the plumbing/matrix-val-dist branch July 5, 2022 19:02
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

calc_expectation returns additional dimension
4 participants