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

Removing time flipping and time flow state #570

Merged
merged 28 commits into from
Apr 23, 2020

Conversation

sbenthall
Copy link
Contributor

Addresses #569

Refactoring so that models always store time varying data in time-forward format.

'Backwards time' is to be a matter internal to dynamic programming solvers.

It would also be possible to have a report method that reports this data in a time backwards way if that's important for presentation.

There are a few failing tests in this PR at the moment--honestly fewer than I expected.
They seem mostly due to lifecycle and cyclical models.
I wonder if @mnwhite has any ideas about how time flipping interacts with those cases so that I can go make the necessary corrections.

@llorracc
Copy link
Collaborator

As long as you are tackling this, I'd propose that you investigate whether there might be some standardized data structures for representing time-varying data. For example, for time series econometrics tools, there is often a way to represent data that "understands" that an interval (of, say, a year" can be divided into subintervals (like, say, quarters, months, weeks, days) which should then aggregate back up to the full interval. It would not be hard to persuade me that using such structures would be overkill from our perspective. But maybe there are lightweight standards that would work well.

This is actually something of a substantive issue, because one of the challenges facing the literature at the moment is the choice between the use of continuous time methods and discrete time methods. Any choices that we can make that would ease the comparison between continuous time and discrete time solutions might prove useful down the road.

@sbenthall
Copy link
Contributor Author

@llorracc There's an easy answer to your question. It's pandas

https://pandas.pydata.org/pandas-docs/stable/user_guide/timeseries.html

It's very widely used. I've used it in both finance and communications analytics.

I'm glad you brought this up. Pandas could be a useful backend for a lot of things. It has some nice integration with Jupyter notebooks and is considered part of the same software ecosystem.

I'm trying to make incremental steps so that we can do thorough testing and review before changing core functionality. So I don't think I'll swap in a Pandas backend in this PR or issue. But I'll keep it in mind if it looks viable in future work.

@llorracc
Copy link
Collaborator

Great, I've used pandas a good bit already, but did not realize that it has special facilities for handling time series data (which is I think what you are saying?)

@sbenthall
Copy link
Contributor Author

Yes. I've linked to its documentation on that topic.

@sbenthall
Copy link
Contributor Author

Ok, I have traced the discrepancies in one of the tests:

The test in test_ConsIndShockSolverBasic is failing because there is a difference in solver.vPfuncNext when the time reversal logic is removed (as per this PR).

@sbenthall
Copy link
Contributor Author

This backs up to a difference in LifecycleExample.solution[1].vPfunc

@sbenthall
Copy link
Contributor Author

Ah, ok, now I see...

ConsIndShockSolver, and I assume most of the other solvers, have been written with the assumption that the time varying parameters will be flipped backwards at the time when solve() is run.

Wow.

@mnwhite
Copy link
Contributor

mnwhite commented Mar 16, 2020 via email

@sbenthall
Copy link
Contributor Author

Ah, that's a relief. I must have been misreading earlier.

@sbenthall sbenthall requested a review from mnwhite March 16, 2020 16:28
@sbenthall
Copy link
Contributor Author

I'm requesting a review from @mnwhite .

The tricky part was the loop in solveOneCycle.
I've added some more tests for this code to make sure it's tight.

@sbenthall sbenthall changed the title [WIP] Removing time flipping and time flow state Removing time flipping and time flow state Mar 16, 2020
@llorracc
Copy link
Collaborator

@mnwhite: It seems like this might break a bunch of things?

@sbenthall
Copy link
Contributor Author

@llorracc This PR does not break anything currently covered by the automated tests in HARK.

It is possible that some custom code built on top of HARK might break.
This is always a risk when changing HARK, and why it is a good idea to pin downstream code that is not being actively maintained to a stable release version.

@sbenthall sbenthall requested a review from llorracc April 3, 2020 13:53
@sbenthall
Copy link
Contributor Author

Any objection to my merging this PR? It has been sitting for 12 days.

@mnwhite
Copy link
Contributor

mnwhite commented Apr 7, 2020 via email

@mnwhite mnwhite merged commit 78d1981 into econ-ark:master Apr 23, 2020
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.

3 participants