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

simplify checks for time varying solveOnePeriod, fixes #496 #498

Merged
merged 3 commits into from
Mar 5, 2020

Conversation

sbenthall
Copy link
Contributor

This PR resolves #496 by simplifying the code around multiple solveOnePeriod methods:

  • The check is performed in only one place (less overhead on user reading and understanding the code)
  • It does not require special use of the time_vary list (less overhead on user building on the code)

@sbenthall sbenthall requested a review from mnwhite February 9, 2020 00:22
@sbenthall
Copy link
Contributor Author

Regarding automated tests for the functionality touched by this PR:

There are two cases:

  • The first case, where solveOnePeriod is a function, is tested by this test: https://github.com/econ-ark/HARK/blob/master/HARK/tests/test_core.py#L100
  • The second case, where solveOnePeriod is subscriptable:
    • has never been used
    • there appears to be no example in the literature of how it would be used
    • theoretically, if there were a case where it could be used, it would be possible to implement it otherwise by having solveOnePeriod be a function that had several cases.

I'm hesitant to write a test for this functionality that simply may not be necessary or desirable in the first place.

@sbenthall sbenthall merged commit b1b14ad into econ-ark:master Mar 5, 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.

simplify code around time-varying solveOnePeriod methods
1 participant