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

Loading default parameters into class during initialization #466

Merged
merged 11 commits into from Jan 9, 2020

Conversation

sbenthall
Copy link
Contributor

This PR is for progress on #446 -- parameter management.

One attractive possibility is that each Model class has default parameters that can be overridden during initialization.

Commit 5c6afef models a way to do this with minimal change to the library for PerfForesightConsumerType:

  • The same dictionary of parameters from is used as before (this dictionary can be moved later)
  • It is accessed and copied during PerfForesightConsumerType initialization
  • It is update()ed with the keyword arguments to the initialize
  • This is then passed through to the superclass constructor as initialization proceeds.

In other work on this issue, I'll set this up for the other Model/Type classes. I wanted to put this early PR version out there because I wonder if others think this is a good change.

@sbenthall sbenthall requested a review from MridulS January 3, 2020 20:17
@project-bot project-bot bot added this to Needs Triage in Issues & PRs Jan 3, 2020
@sbenthall sbenthall changed the title [WIP] Loading default parameters into class during initialization Loading default parameters into class during initialization Jan 6, 2020
@sbenthall
Copy link
Contributor Author

This pull request is now ready for review. Requested from @MridulS

@@ -965,7 +966,7 @@ class GenIncProcessConsumerType(IndShockConsumerType):
solution_terminal_ = ConsumerSolution(cFunc=cFunc_terminal_, mNrmMin=0.0, hNrm=0.0, MPCmin=1.0, MPCmax=1.0)
poststate_vars_ = ['aLvlNow', 'pLvlNow']

def __init__(self, cycles=1, time_flow=True, **kwds):
def __init__(self, cycles=0, time_flow=True, **kwds):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason for this switch?

@@ -509,7 +510,7 @@ class MedShockConsumerType(PersistentShockConsumerType):
'''
shock_vars_ = PersistentShockConsumerType.shock_vars_ + ['MedShkNow']

def __init__(self,cycles=1,time_flow=True,**kwds):
def __init__(self,cycles=0,time_flow=True,**kwds):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing


# Make a dictionary for the "explicit permanent income" idiosyncratic shocks model
init_explicit_perm_inc = copy(init_idiosyncratic_shocks)
init_explicit_perm_inc['pLvlPctiles'] = pLvlPctiles
init_explicit_perm_inc['PermGroFac'] = [1.0] # long run permanent income growth doesn't work yet
init_explicit_perm_inc['cycles'] = cycles
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably I am missing something here but why these changes?

@llorracc
Copy link
Collaborator

llorracc commented Jan 6, 2020

Can you trace back to the PR where the changes were made?

Looks to me as though the default is being changed from finite to infinite horizon models. It should have been infinite horizon all along, so if that is what is being done it make sense -- puzzle is why it wasn't infinite horizon all along.

@sbenthall
Copy link
Contributor Author

@MridulS Those changes were made because:

  • the default parameters in ConsumerParameters.py set cycles=0
  • The classes had __init__(cycles=1), but this was overwritten by the loading of the parameter dictionary at runtime
  • When the superclass __init__ is being called, it is both passing through cycles=cylces as a named argument and also passing **kwds through
  • Without these changes, the Python interpreter complains that the superclass __init__ is getting 2 different cycles arguments on the same call.

As @llorracc says, the infinite horizon was being passed through to override the finite horizon anyway. So I made the call that this is set properly in the __init__ and not at all in the parameter dictionary.

@llorracc : I think you may be responded to comments on PR's in email and the lack of context is confusing. The subject line of the email notification names the PR where this discussion is happening. It should say "#466"

@sbenthall
Copy link
Contributor Author

Ok to merge #466?

@MridulS MridulS self-requested a review January 9, 2020 10:04
@MridulS MridulS merged commit ad83315 into econ-ark:master Jan 9, 2020
Issues & PRs automation moved this from Needs Triage to Done Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Issues & PRs
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants