-
-
Notifications
You must be signed in to change notification settings - Fork 199
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
Distribute parameter dictionaries to nearby class definitions #527
Conversation
… PerfForesightConsumerType class
OK, this PR is now ready for review. I expect @mnwhite will want to take a look. The general idea is that it has moved the default ConsumptionSaving model parameters from ConsumerParams (which doesn't exist any more, in this PR) to objects that are defined much closer to the AgentTypes where the parameters are primarily used. These objects are exposed through their respective module interfaces, so they are still used elsewhere. The main benefits to this are:
While admittedly a rather small and mainly cosmetic change, it's working towards a more systematic way of documenting, inheriting, and accessing parameters. With a bit more work, the new parameters for each sub-AgentType can be properly documented, and the parameters list can be stored in a way that can be easily exported for viewing. Most software tooling (like Sphinx) will assume default arguments are near their classes. We won't have to fight the documentation engine to list these parameters. |
@sebastian Benthall <[email protected]> I like this direction very much. It's
compelling as the way we ought to move. My one question is, before we
merge the PR, we need to include with it changes to all the DemARK and
REMARK repos that rely on the current structure. I wonder whether, rather
than one giant PR for DemARK, REMARK, and HARK, there might be any way to
break it down into easily digestible steps? LIke, are there ways that
DemARK/REMARK content could be revised so that it would work with both the
old and the new way of doing things; maybe to HARK.utilities we could add
some testing facility and have some boilerplate if ... then constructions
that test whether we are using the old or the new way of doing things...
…On Fri, Feb 21, 2020 at 9:18 PM Sebastian Benthall ***@***.***> wrote:
OK, this PR is now ready for review.
I expect @mnwhite <https://github.com/mnwhite> will want to take a look.
The general idea is that it has moved the default ConsumptionSaving model
parameters from ConsumerParams (which doesn't exist any more, in this PR)
to objects that are defined much closer to the AgentTypes where the
parameters are primarily used.
These objects are exposed through their respective module interfaces, so
they are still used elsewhere.
The main benefits to this are:
- Because the parameter objects are defined closer to where they are
used, it's easier to see what the input variables to an AgentType are
without having to flip between files.
- I've made it stylistically more succinct: this commit removes 60
lines of code from the repository. There's one fewer module (no more
ConsumerParams)
- Now, not every new type has to touch the additional ConsumerParams
file; a new model can be wholy contained in its own module.
While admittedly a rather small and mainly cosmetic change, it's working
towards a more systematic way of documenting, inheriting, and accessing
parameters. With a bit more work, the new parameters for each sub-AgentType
can be properly documented, and the parameters list can be stored in a way
that can be easily exported for viewing. Most software tooling (like
Sphinx) will assume default arguments are near their classes. We won't have
to fight the documentation engine to list these parameters.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#527?email_source=notifications&email_token=AAKCK76UFF5LHTBTYLDMQDTREAZHZA5CNFSM4KV6JA42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMT6WPQ#issuecomment-589818686>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKCK747RKAVH5HGP5VFKCLREAZHZANCNFSM4KV6JA4Q>
.
--
- Chris Carroll
|
I think I see the issue you are raising. I believe the non-backwards compatible changes will be cases where a DemARK or REMARK imports one of the parameter objects (e.g. I would not advise having code dynamically choose the import path to ensure backwards compatibility. That gets very messy. Rather, I think the way to handle this is through skilled release management. Something like this:
These changes would secure the downstream dependencies from breaking due to changes in HARK. Next: pull in these changes to parameter handling to HARK Finally: in the downstream projects, work on PRs that update the HARK dependency and fix any problems that result from that change. |
Some changes needed to get this to work:
|
The failing I must have missed something when moving the parameter dictionaries... |
This looks good to me. I like the new format. I'm happy to merge if no one objects. |
@sbenthall this will probably break demarks and remarks too. Have you looked into that? |
@sbenthall examples too. |
@sbenthall, in the Portfolio model, we never had init_portfolio in ConsumerParams.py, it was defined in the portfolio model itself. Is that the preferred way of doing things going forward, or should @mnwhite put init_portfolio in ConsumerParameters.py? |
ConsumerParamters.py is gone. All default/example dictionaries are now in
the model files.
…On Tue, Mar 17, 2020 at 2:03 PM Christopher Llorracc Carroll < ***@***.***> wrote:
OK, this PR is now ready for review.
I expect @mnwhite <https://github.com/mnwhite> will want to take a look.
The general idea is that it has moved the default ConsumptionSaving model
parameters from ConsumerParams (which doesn't exist any more, in this PR)
to objects that are defined much closer to the AgentTypes where the
parameters are primarily used.
These objects are exposed through their respective module interfaces, so
they are still used elsewhere.
The main benefits to this are:
- Because the parameter objects are defined closer to where they are
used, it's easier to see what the input variables to an AgentType are
without having to flip between files.
- I've made it stylistically more succinct: this commit removes 60
lines of code from the repository. There's one fewer module (no more
ConsumerParams)
- Now, not every new type has to touch the additional ConsumerParams
file; a new model can be wholy contained in its own module.
While admittedly a rather small and mainly cosmetic change, it's working
towards a more systematic way of documenting, inheriting, and accessing
parameters. With a bit more work, the new parameters for each sub-AgentType
can be properly documented, and the parameters list can be stored in a way
that can be easily exported for viewing. Most software tooling (like
Sphinx) will assume default arguments are near their classes. We won't have
to fight the documentation engine to list these parameters.
- Now, not every new type has to touch the additional ConsumerParams
file; a new model can be wholy contained in its own module.
@sbenthall <https://github.com/sbenthall>, in the Portfolio model, we
never had init_portfolio in ConsumerParams.py, it was defined in the
portfolio model itself. Is that the preferred way of doing things going
forward, or should @mnwhite <https://github.com/mnwhite> put
init_portfolio in ConsumerParameters.py?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#527 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFLKQA4QI456STT3MB3RH63PRANCNFSM4KV6JA4Q>
.
|
@sbenthall this is a major change, can you add it to v0.10.5 let's add any major/minor change to the codebase to this just before a merge, as it will make our release process quick. |
Made #580 to address the |
I get it for REMARKs but DemARKs are supposed to work with the master branch of HARK. We don't want users to do |
I agree with @MridulS: We want DemARKs (and examples) to always work with the latest version of HARK. If we make a change that breaks a DemARK, but really want to make that change, then I think we will want to remove that DemARK to a "release-10.4-broke-me" branch until fixed. I guess that ideally we would want to version-control the DemARK itself. We could do that by having an initialization cell in every DemARK that just tested whether the version of HARK being used was greater than the version of DemARK, and if so halting with a message that the person should pull down the updated DemARK version. |
I've made an issue to discuss this further here econ-ark/DemARK#108 |
* Add parameter movement to major changes for 0.10.5, see #527 * Update CHANGELOG.md Co-authored-by: Mridul Seth <[email protected]>
A PR for the next proposed stage of #446
Currently, all default model parameters are defined in
ConsumerParameters.py
.Thus, in order to know the default parameters for a model type, one has to look in a separate file.
This is very nonstandard for default parameter values in Python.
Moreover, while the old means of having models "inherit" parameters made this structure necessary or at least convenient, now that models inherit the default parameters of their superclasses at initialization, it makes more sense to have parameters be defined near to where they are used.
This PR is for ongoing work to see what this different way of laying out the default parameters would look like.