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

Fix related params #2532

Merged
merged 8 commits into from
Feb 1, 2021
Merged

Fix related params #2532

merged 8 commits into from
Feb 1, 2021

Conversation

hdoupe
Copy link
Collaborator

@hdoupe hdoupe commented Jan 13, 2021

Resolves #2531 using transaction method added in PSLmodels/ParamTools#123.

TODO:

  • Evaluate performance issues to make sure the extra validation step doesn't cause too much of a performance hit.
  • Merge the ParamTools PR and do a release.

@hdoupe hdoupe marked this pull request as draft January 13, 2021 17:09
@hdoupe hdoupe marked this pull request as ready for review January 26, 2021 01:54
@hdoupe
Copy link
Collaborator Author

hdoupe commented Jan 26, 2021

I took a look at a simple bench mark that adjust a parameter over multiple years EITC_c, another parameters indexed status (STD), and the indexing offset (parameter_indexing_CPI_offset) to check the performance. The extra validation on parameters that were not adjusted caused about a 150 ms slow down from 2.05 seconds or about a 7% slow down on validation. This is pretty small and likely will not affect users unless they are using it in a web request-response context.

Before:
Screenshot from 2021-01-25 18-15-08

After:
Screenshot from 2021-01-25 21-03-43

@hdoupe
Copy link
Collaborator Author

hdoupe commented Jan 26, 2021

I'll fix the ParamTools warning in a follow up PR. I started working on this here while developing the new API in ParamTools. This may improve the performance a slight bit, too.

- Mostly reformats docs to be rendered by jupyter books
- Clarifies some grammer/long sentences
- Removes inherited ParamTools methods from the docs
  and replaces them with a link to the ParamTools docs
@hdoupe
Copy link
Collaborator Author

hdoupe commented Jan 26, 2021

Latest commit d4e706f cleans up some of the parameters documentation:

  • Mostly reformats docs to be rendered by jupyter books
  • Clarifies some grammar/long sentences
  • Removes inherited ParamTools methods from the docs and replaces them with a link to the ParamTools docs

@MattHJensen there are a lot of changes in this commit, most of them are just formatting. I did update the docstrings in a couple places and I'll drop some comments to let you know where.

taxcalc/parameters.py Show resolved Hide resolved
taxcalc/parameters.py Show resolved Hide resolved
taxcalc/parameters.py Show resolved Hide resolved
taxcalc/parameters.py Show resolved Hide resolved
@MattHJensen
Copy link
Contributor

Thanks a lot, @hdoupe. This looks good to me. Merging.

@MattHJensen MattHJensen merged commit 2142329 into PSLmodels:master Feb 1, 2021
@hdoupe hdoupe deleted the fix-related-params branch February 1, 2021 13:59
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.

Error when simulataneously adjusting related parameters and their indexed status
2 participants