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

Add more parameter types to Parameters #1387

Merged
merged 10 commits into from
Jun 28, 2024

Conversation

Mv77
Copy link
Contributor

@Mv77 Mv77 commented Feb 22, 2024

I have been playing with the core.Parameters class and really like it. One limitation is that it restricts the class of parameters that can be time-varying. This PR adds support for a few more classes: booleans, distributions, and functions.

You can imagine passing time-varying parameters of this class if, for instance you want:

  • To indicate whether an agent is retired = True | False for some meaningful change in its income process.
  • To have a time varying distribution for income shocks. Maybe this period they are normal, and the next they are lognormal.
  • To have a time varying tax function.

I have used this class in my ongoing work and have been very happy with it.

  • Tests for new functionality/models or Tests to reproduce the bug-fix in code.
  • Updated documentation of features that add new functionality.
  • Update CHANGELOG.md with major/minor changes.

@Mv77
Copy link
Contributor Author

Mv77 commented Feb 22, 2024

@alanlujan91 should this change be reflected in other parts of the code like, say,

self._parameters: Dict[str, Union[int, float, np.ndarray, list, tuple]] = {}
?

@Mv77 Mv77 requested a review from alanlujan91 February 22, 2024 23:54
@Mv77
Copy link
Contributor Author

Mv77 commented Feb 22, 2024

Not sure this needs a test?

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.69%. Comparing base (bbb07a5) to head (7417c54).

❗ Current head 7417c54 differs from pull request most recent head 8fb9040. Consider uploading reports for the commit 8fb9040 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1387      +/-   ##
==========================================
+ Coverage   71.53%   71.69%   +0.15%     
==========================================
  Files          83       84       +1     
  Lines       13938    13939       +1     
==========================================
+ Hits         9971     9993      +22     
+ Misses       3967     3946      -21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alanlujan91 alanlujan91 requested a review from mnwhite February 26, 2024 14:59
@alanlujan91
Copy link
Member

@MridulS is there a way for pre-commit to automatically upload changes?

@MridulS
Copy link
Member

MridulS commented Feb 26, 2024

There is pre-commit.ci but it makes changes to the PR branch and you need to make sure the contributors are pulling in all changes before making any new changes locally. I would just strongly suggest to new contributors to use pre-commit locally :)

@alanlujan91
Copy link
Member

well, somehow my pre-commit is different than the one in github actions?? @MridulS

@Mv77
Copy link
Contributor Author

Mv77 commented Apr 26, 2024

MAC hates me. And I hate it too.
It's the same tests as in #1415

Will come back to this when that's merged

@sbenthall
Copy link
Contributor

This PR was blocked by some unrelated logistical thing.
Now its only conflict is the CHANGELOG.

Can this be fixed and merged now?

@mnwhite
Copy link
Contributor

mnwhite commented Jun 25, 2024 via email

@mnwhite
Copy link
Contributor

mnwhite commented Jun 28, 2024

GitHub is hanging on its automatic merge check. I'm trying to get this in now.

@mnwhite mnwhite merged commit 7f5091f into econ-ark:master Jun 28, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants