Labeled income process

[Mv77]

This PR changes the income process constructor in ConsIndShockModel to use @alanlujan91's DiscreteDistributionLabeled class. This:

• Allows for more expressive code (e.g. shocks[0] becomes shocks['PermShk']) when taking expectations.
• Might make the code faster, since it uses Alan's vectorized expectation operator.

I have marked it as WIP because I want to extend the change to portfolio models, but tests are currently passing.

Please ensure your pull request adheres to the following guidelines:

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

[codecov-commenter]

Codecov Report

Base: 73.61% // Head: 73.66% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (1f6a416) compared to base (65c76c8).
Patch coverage: 96.36% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1189      +/-   ##
==========================================
+ Coverage   73.61%   73.66%   +0.05%     
==========================================
  Files          73       73              
  Lines       11998    12024      +26     
==========================================
+ Hits         8832     8858      +26     
  Misses       3166     3166              

Impacted Files	Coverage Δ
HARK/ConsumptionSaving/ConsRiskyAssetModel.py	41.03% <80.00%> (+0.43%)	⬆️
HARK/distribution.py	83.10% <88.88%> (+0.30%)	⬆️
HARK/ConsumptionSaving/ConsIndShockModel.py	86.50% <100.00%> (ø)
HARK/ConsumptionSaving/ConsMarkovModel.py	85.83% <100.00%> (ø)
HARK/ConsumptionSaving/ConsPortfolioModel.py	89.88% <100.00%> (+0.13%)	⬆️
...RK/ConsumptionSaving/tests/test_ConsMarkovModel.py	86.56% <100.00%> (ø)
...K/ConsumptionSaving/tests/test_modelcomparisons.py	98.46% <100.00%> (ø)
HARK/tests/test_distribution.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.

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

[alanlujan91]

Should this be np.random.RandomState(value)?

[Mv77]

I am not sure? What does this property do?

I wrote as it is because I thought that the get and set method should deal with exactly the same object. Since the getter is

HARK/HARK/distribution.py

Lines 1188 to 1193 in daf2afd

	@property
	def RNG(self):
	"""
	Returns the distribution's random number generator.
	"""
	return self.dataset.RNG

I wrote the setter to the RNG property directly. What do you think?

[alanlujan91]

I think this is how we initialize a random state with a seed. When we initialize a DiscreteDsitribution object, we set RNG as follows

HARK/HARK/distribution.py

Line 1079 in daf2afd

attrs["RNG"] = np.random.RandomState(seed)

so I guess we would have to do dist.RNG = np.random.RandomState(seed).

Actually I guess this isn't a problem, you just have to be more explicit that the value parameter should be of type RandomState. Maybe just a type check?

But also if we have a setter for dist.seed = value it should also generate a new RNG.

[Mv77]

I like the typecheck solution.

You did not add a setter for the seed property (I think?) Should I create one? Would that go in a different PR?

[alanlujan91]

It might be useful to have a seed setter. I think it would be fine to go in this PR.

[Mv77]

Alrighty, added a typecheck.

I could not remember why I had to add this setter in the first place, but just ran some tests and found out. The reason was that methods like initialize_sim call Distribution.reset() on things like the income distribution. That method is

HARK/HARK/distribution.py

Lines 24 to 31 in 20645e2

	def reset(self):
	"""
	Reset the random number generator of this distribution.
	Parameters
	----------
	"""
	self.RNG = np.random.RandomState(self.seed)

So without a setter, it failed

[alanlujan91]

@Mv77 is this done? I can merge it once you resolve conflicts

[Mv77]

Not yet!

…

On Wed, Dec 7, 2022, 2:09 PM alanlujan91 ***@***.***> wrote: @Mv77 <https://github.com/Mv77> is this done? I can merge it once you resolve conflicts — Reply to this email directly, view it on GitHub <#1189 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGTULS4KZYPJHYP4FKPLOTTWMDOIPANCNFSM6AAAAAASIN2Z3I> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Mv77]

This is ready for review and merge if it passes review.

@llorracc I checked locally that it does not break DemARKs. It's much harder to know for REMARKs, but afaik version control for REMARKs is in the works.

[llorracc]

Yes, version control for REMARKs is in the works.

I've asked @sbenthall to work on preparing for a new release (0.13) by the end of this month, and I hope by that time we will have nailed down the status of all the REMARKs: Either pinned to earlier versions of HARK, or added to the tests that are run when new PRs are submitted.

PS. I'm a bit puzzled by the combination of the "Needs: Revision" and the "Ready-To-Merge" labels. Could you clarify what revisions you view as needed? I guess it's hard to see how to keep track of whether we get those revisions done after the PR is merged. Maybe the right approach is to make an "issue" containing the revisions that are needed, and link to the issue in this discussion of the merge request?

[Mv77]

What I wanted to express was "This is ready from my point of view. It would be good to get someone to look at it. If that someone approves, they can go ahead and merge without further questions."

[llorracc]

OK, I guess the right "someone" is @alanlujan91

Alan, go for it!

[sbenthall]

I've removed the "Needs:revision" label.