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

Update RNG #1193

Merged
merged 7 commits into from
Dec 28, 2022
Merged

Update RNG #1193

merged 7 commits into from
Dec 28, 2022

Conversation

alanlujan91
Copy link
Member

@alanlujan91 alanlujan91 commented Dec 14, 2022

Since Numpy version 1.17.0 scipy introduced Generator as a replacement to RandomState.

This PR updates HARK to use Generator as recommended in https://numpy.org/doc/stable/reference/random/index.html#quick-start

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

@alanlujan91
Copy link
Member Author

Few errors remain regarding precision given a new RNG.

@llorracc
Copy link
Collaborator

Are these the kinds of errors that would disappear if we scaled back our degree of precision? If so, that's the solution.

@alanlujan91
Copy link
Member Author

alanlujan91 commented Dec 14, 2022

TODO: level of precision to 3 or 4 decimals global default

@alanlujan91 alanlujan91 linked an issue Dec 14, 2022 that may be closed by this pull request
@alanlujan91
Copy link
Member Author

alanlujan91 commented Dec 15, 2022

FIND: assertAlmostEqual((.), (\d.\d{5}))
REPLACE: assertAlmostEqual($1, $2, places = HARK_PRECISION)

Copy link
Collaborator

@llorracc llorracc left a comment

Choose a reason for hiding this comment

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

@alanlujan91,

Thanks for this! Some people dream of winning or beauty or fame. It has long been my dream to reduce the excessive precision of our numerical tests. This is a big step in that direction!

Comments:

  1. I think we might want at least a couple of different precision defaults
    • HARK_PRECISION_NORMALIZED for variables that are normalized by permanent income
    • HARK_PRECISION_ZERO for variables that should be approximately zero
  2. Sadly, your PR ruins some beautiful formatting in which explanatory comments were perfectly aligned with each other on the right hand border of some multiline code snippets. Now everything is mooshed to the left to be one space beyond the last character of the actual instruction.
    • This is probably the fault of VSCode or Black or Flake8. It's not worth a lot of effort to fix -- but is worth a little. So if it only takes a little, please do it.

@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2022

Codecov Report

Base: 73.62% // Head: 73.61% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (1506245) compared to base (86d5c62).
Patch coverage: 75.29% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1193      +/-   ##
==========================================
- Coverage   73.62%   73.61%   -0.02%     
==========================================
  Files          72       73       +1     
  Lines       11973    11998      +25     
==========================================
+ Hits         8815     8832      +17     
- Misses       3158     3166       +8     
Impacted Files Coverage Δ
HARK/ConsumptionSaving/ConsAggShockModel.py 89.64% <ø> (ø)
HARK/ConsumptionSaving/ConsGenIncProcessModel.py 82.45% <ø> (ø)
HARK/ConsumptionSaving/ConsPrefShockModel.py 79.68% <ø> (ø)
HARK/ConsumptionSaving/ConsRepAgentModel.py 100.00% <ø> (ø)
HARK/ConsumptionSaving/ConsRiskyAssetModel.py 40.59% <ø> (ø)
HARK/estimation.py 0.00% <0.00%> (ø)
HARK/utilities.py 30.09% <22.80%> (+0.46%) ⬆️
...RK/ConsumptionSaving/tests/test_ConsMarkovModel.py 86.56% <60.00%> (ø)
HARK/interpolation.py 43.34% <60.00%> (ø)
HARK/ConsumptionSaving/ConsMarkovModel.py 85.83% <66.66%> (ø)
... and 19 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@llorracc
Copy link
Collaborator

@alanlujan91 let's discuss merging this in tomorrow's meeting.

@alanlujan91 alanlujan91 added the Ready-To-Merge Has been reviewed and when branch is updated and checks pass it should be merged label Dec 21, 2022
@llorracc llorracc merged commit 23ed315 into econ-ark:master Dec 28, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reducing pytest approx_equals sensitivity to only 6 decimal places
4 participants