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.

Sign up for GitHub

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

Jump to bottom

track_vars cannot track PrefShkNow #808

Closed
sbenthall opened this issue Aug 20, 2020 · 10 comments · Fixed by #812
Closed

track_vars cannot track PrefShkNow #808

sbenthall opened this issue Aug 20, 2020 · 10 comments · Fixed by #812
Assignees
@sbenthall
Milestone
0.x.y

Comments

@sbenthall
Copy link
Contributor

Describe the bug
Adding PrefShkNow to track_vars results in broken history records.

To Reproduce

Run this code:
https://gist.github.com/sbenthall/e398fd3c389a9cc094974c397e49b160

See this output:

python pref_test.py 
0.6904681186891202 0.69046812
/home/sb/projects/econ-ark/HARK/HARK/interpolation.py:1721: RuntimeWarning: All-NaN slice encountered
  y = self.compare(fx,axis=1)
[[nan nan nan ... nan nan nan]
 [nan nan nan ... nan nan nan]
 [nan nan nan ... nan nan nan]
 ...
 [nan nan nan ... nan nan nan]
 [nan nan nan ... nan nan nan]
 [nan nan nan ... nan nan nan]]
[[nan nan nan ... nan nan nan]
 [nan nan nan ... nan nan nan]
 [nan nan nan ... nan nan nan]
 ...
 [nan nan nan ... nan nan nan]
 [nan nan nan ... nan nan nan]
 [nan nan nan ... nan nan nan]]

Expected behavior

When PrefShkNow is removed from track_vars, and the corresponding print statement is removed, the numerical values of cNrmNow are displayed.

$ python pref_test.py 
0.6904681186891202 0.69046812
[[1.31820519 1.13599726 1.19117047 ... 1.15580804 0.92238984 0.79139412]
 [1.75227515 0.87249487 1.03166147 ... 1.09467154 1.04217198 0.81947656]
 [1.6132044  1.21100077 1.10807752 ... 1.04517571 0.97166597 0.93927158]
 ...
 [1.41013941 0.98367178 0.99266247 ... 1.19269696 1.00552796 1.02108079]
 [1.19004436 1.32309231 1.52353897 ... 1.10650642 0.87607768 0.92796219]
 [1.07060879 0.94171512 1.06713576 ... 0.94867343 0.8665761  0.93075895]]

I expect a similar thing when adding PrefShkNow to be tracked.
@sbenthall sbenthall added this to the 0.x.y milestone Aug 20, 2020
@mnwhite
Copy link
Contributor

mnwhite commented Aug 20, 2020 via email

@sbenthall
Copy link
Contributor Author

That worked....

Let's figure out how to make this documented/obvious and fail more gracefully.

@sbenthall sbenthall self-assigned this Aug 20, 2020
@sbenthall
Copy link
Contributor Author

Interesting.
WITH makeShockHistory, and WITHOUT the addition of PrefShkNow to track_vars, it also works.

So, this is specifically a problem with how adding it to track_vars interacts with makeShockHistory

@sbenthall
Copy link
Contributor Author

Hmm. So, one thing I'm noticing is that a matrix full of nans is what you initialize each shock variable with at the beginning of makeShockHistory

https://github.com/econ-ark/HARK/blob/master/HARK/core.py#L481

Another is that makeShockHistory calls initializeSim(), but in the test code is called before initializeSim().

initializeSim() clears the history:
https://github.com/econ-ark/HARK/blob/master/HARK/core.py#L423

which I think would erase everything already done by makeShockHistory.

But if I do NOT call initializeSim() after makeShockHistory(), I get this error:

Traceback (most recent call last):
  File "pref_test.py", line 20, in <module>
    agent.makeShockHistory()  # This is optional
  File "/home/sb/projects/econ-ark/HARK/HARK/core.py", line 687, in simulate
    self.simOnePeriod()
  File "/home/sb/projects/econ-ark/HARK/HARK/core.py", line 445, in simOnePeriod
    self.getMortality()  # Replace some agents with "newborns"
  File "/home/sb/projects/econ-ark/HARK/HARK/core.py", line 520, in getMortality
    who_dies = self.history['who_dies'][self.t_sim,:]
IndexError: index 10 is out of bounds for axis 0 with size 10

Which is dealing with the special case in getMortality which expects self.read_shocks == True

https://github.com/econ-ark/HARK/blob/master/HARK/core.py#L515-L516

@mnwhite
Copy link
Contributor

mnwhite commented Aug 21, 2020 via email

@sbenthall
Copy link
Contributor Author

Hmmm.

Ok, so let be get this straight...

  1. Initialize agent.

    • read_shocks = False

  2. makeShockHistory

    • calls initalizeSim():
      • t_sim = 0
      • clears history
    • read_shocks = True
    • increments t_tim to T_sim - 1
    • puts the shock values into history

It's necessary then to call initializeSim again:

  • this sets t_sim = 0 again.
  • it also calls clearHistory()

What does clearHistory() do?

https://github.com/econ-ark/HARK/blob/master/HARK/core.py#L685-L698

It erases the history of all the track_vars.

This explains the bug!

@sbenthall
Copy link
Contributor Author

So the question now is how to fix it.

From where I'm sitting, the use case fo track_vars is very different from the use case of makeShockHistory(). They should not be using the same data structure (this history dictionary, which used to be *_hist object attributes) for their data.

What makes makeShockHistory possible is precisely that the shocks are exogenous, and so can be precomputed. This relates to what is being discussed in #798. They can be precomputed because there are not dependent on endogenous variables; with respect to the partial ordering of computation, they are always at the root.

A simple fix would be to have makeShockHistory use a different dictionary for storage, shock_hist.

@sbenthall sbenthall mentioned this issue Aug 21, 2020
Merged
2 tasks
sbenthall added a commit to sbenthall/HARK that referenced this issue Aug 21, 2020
@sbenthall
adds the econ-ark#808 shock_history fix to the changelog
787fb86
@mnwhite
Copy link
Contributor

mnwhite commented Aug 21, 2020 via email

@sbenthall
Copy link
Contributor Author

Yes, that's correct.
But would you rather tell the user:

  • If you want to track the history of a variable, then add it to 'track_vars', unless it's a shock variable and you're calling makeShockHistory, in which case, don't worry, the shock history is maintained for you...

or...

  • If you want to track the history of a variable, add it to track_vars

@mnwhite
Copy link
Contributor

mnwhite commented Aug 21, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sbenthall sbenthall

Labels
None yet
Projects
None yet
Milestone
0.x.y
Development

Successfully merging a pull request may close this issue.

shock_history instead of history namespace for read_shocks sbenthall/HARK
2 participants
@sbenthall @mnwhite