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

Why doesn't checkConditions() work in ConsPortfolioModel? #416

Open
llorracc opened this issue Oct 28, 2019 · 10 comments
Open

Why doesn't checkConditions() work in ConsPortfolioModel? #416

llorracc opened this issue Oct 28, 2019 · 10 comments

Comments

@llorracc
Copy link
Collaborator

My understanding is that ConsPortfolioModel should just build on top of everything in ConsIndShockConsumerType, which would include the .checkConditions() method. While the method seems to be there, e.g. in the pcct instance of PortfolioConsumerType in ConsPortfolioModelDoc, it doesn't do anything. (I was looking at this because the conditions actually should be adjusted when the rate of return is risky).

@pkofod @mnwhite probably know the answer ...

@sbenthall sbenthall self-assigned this Mar 11, 2020
@sbenthall sbenthall added this to the 0.x.y milestone Mar 12, 2020
@sbenthall
Copy link
Contributor

There are two AgentTypes defined in ConsPortfolioModel

  1. PortfolioConsumerType
  2. LogNormalPortfolioConsumerType

(1) subclasses IndShockConsumerType, but its initializer does not use super() to get the superclass; instead it calls the initializer of PerfForesightConsumerType. I expect that this is an error.

PerfForesightConsumerType.__init__(

(2) Subclasses PerfForesightConsumerType and then uses the PerfForesightConsumerType initializer.

class LogNormalPortfolioConsumerType(PortfolioConsumerType):

If you can confirm the desired behavior here (subclassing ConsIndShockConsumerType properly at (1).... any change to (2)?) I'll work these changes in to #568

@mnwhite
Copy link
Contributor

mnwhite commented Mar 13, 2020 via email

@sbenthall
Copy link
Contributor

Ok. I'll reassign this issue to you, since it sounds like it will best be checked/solved in your rewrite.
I'm not 100% sure that fixing the superclass initializer issue will fix checkConditions(), so you might want to double check that. (Ideally, with an automated test for it)

@sbenthall sbenthall assigned mnwhite and unassigned sbenthall Mar 13, 2020
@mnwhite
Copy link
Contributor

mnwhite commented Mar 13, 2020 via email

@llorracc
Copy link
Collaborator Author

llorracc commented Mar 13, 2020 via email

@mnwhite
Copy link
Contributor

mnwhite commented Mar 13, 2020 via email

@llorracc
Copy link
Collaborator Author

llorracc commented Mar 13, 2020 via email

@MridulS
Copy link
Member

MridulS commented Aug 24, 2020

@Mv77

@MridulS
Copy link
Member

MridulS commented Aug 24, 2020

@mnwhite
Copy link
Contributor

mnwhite commented Jul 3, 2024

The check_conditions() method has been significantly revised and improved since this issue was last touched. Unfortunately, it's not properly nulled out in child models (like ConsPortfolioModel). This still needs to be fixed, in one of two ways:

  1. Add trivial methods to various models that overwrite check_conditions and do nothing.

  2. Finish the "de-inheritance" overhaul I was working on a couple months ago

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants