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

store shocks in namespace --> #760 #803

Merged
merged 14 commits into from
Aug 27, 2020
Merged

Conversation

sbenthall
Copy link
Contributor

@sbenthall sbenthall commented Aug 18, 2020

Storing the shock values in a namespace dictionary.

  • 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

@mnwhite I wonder if you could help me with this.

I've been able to work out some changes that move the shocks into a namespace -- a dictionary self.shocks on the agent.

I was able to get tests passing on the ConsIndShockModel agent types.

And I was making progress on ConsPrefShockModel and ConsMedShockModel, but then I wanted to make sure I was keeping valid results. So I expanded the test covered to simulated values of cNrmNow (see #802 )

These new tests do not pass.

I can't tell from where I'm sitting whether I should expect different results, maybe because of how the random seeds are drawn, or if I've made a mistake somewhere.

Any ideas?

@sbenthall
Copy link
Contributor Author

An argument seed = self.RNG.randint(0, 2**31-1) needs to be passed in here:
https://github.com/econ-ark/HARK/blob/master/HARK/ConsumptionSaving/ConsPrefShockModel.py#L116-L118

@sbenthall
Copy link
Contributor Author

Ok, it looks like the falling ConsPrefShock test was due to my not properly handling the read_shocks issue.
This PR now has that work, partly enabled by the shock history handling from #812
I can now proceed with the other consumption models

@sbenthall sbenthall requested a review from mnwhite August 22, 2020 00:25
@sbenthall sbenthall changed the title [WIP] store shocks in namespace --> #760 store shocks in namespace --> #760 Aug 22, 2020
@sbenthall
Copy link
Contributor Author

This PR is now ready to review.
It's passing tests locally.

@sbenthall sbenthall closed this Aug 22, 2020
@sbenthall sbenthall reopened this Aug 22, 2020
@sbenthall
Copy link
Contributor Author

(Whoops--closed by accident! Reopened it)

I request the reviewer pay a little extra attention to initializeSim() in MarkovConsumerType.
I had to add a line there to get things to work.

Otherwise, it's mostly just replacing the exogenous shock Now attribute assignments with dictionary inserts, and likewise for access them

@mnwhite
Copy link
Contributor

mnwhite commented Aug 27, 2020

SB: You said to merge this first, before #812, right? This looks good to me, but I wanted to be double sure.

@sbenthall
Copy link
Contributor Author

@mnwhite the other way around. #812 should be, and has been, merged first. this is now good to merge.

@sbenthall sbenthall merged commit 9bff0b1 into econ-ark:master Aug 27, 2020
@mnwhite
Copy link
Contributor

mnwhite commented Aug 27, 2020

Glad I asked.

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