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

Standardize time path output #880

Merged
merged 3 commits into from
Sep 8, 2023
Merged

Conversation

jdebacker
Copy link
Member

This PR standardizes the variables saved to the output dictionary from the time path solution. Previously, some variables had a dimension with length T+S while others had a length of T in the same dimension (as noted in Issue #879). This PR sets the first dimension of all time path output variables to be of length T. This consistency in the length along this dimension will help users more easily use the model output.

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2023

Codecov Report

Merging #880 (a6edd02) into master (0b48a79) will not change coverage.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #880   +/-   ##
=======================================
  Coverage   79.98%   79.98%           
=======================================
  Files          18       18           
  Lines        4157     4157           
=======================================
  Hits         3325     3325           
  Misses        832      832           
Flag Coverage Δ
unittests 79.98% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
ogcore/TPI.py 93.04% <ø> (ø)
ogcore/__init__.py 100.00% <100.00%> (ø)

@jdebacker jdebacker marked this pull request as ready for review September 8, 2023 12:19
@jdebacker
Copy link
Member Author

This PR is ready to go. I ran the full suite of tests locally and found 3 failures:

=========================== short test summary info ============================
FAILED tests/test_txfunc.py::test_txfunc_est[DEP] - assert False
FAILED tests/test_txfunc.py::test_tax_func_loop - assert False
FAILED tests/test_txfunc.py::test_monotone_spline - AttributeError: module 'numpy' has no attribute 'int'.

The first two are expected to fail since I'm using a different version of SciPy than was used for the cached tax function estimates are checking against (and we know different versions produce slightly different parameter estimates). The third failure is because the PyGAM package is incompatible with more recent NumPy versions. If that project would merge PR #321 (which changes just two lines), this test would pass.

@rickecon
Copy link
Member

rickecon commented Sep 8, 2023

@jdebacker. This looks great. Will you please update the version number to 0.10.8 in setup.py and in ogcore/__init__.py. As soon as this is done, I will merge.

@rickecon
Copy link
Member

rickecon commented Sep 8, 2023

@jdebacker. BTW, I keep looking for alternatives to PyGAM for generalized additive models. It is not clear to me if there is a good alternative in scikit-learn. This Jupyter Book "Generalized Additive Models (GAM)" chapter uses the gam module of the statsmodels package (statsmodels.gam), which might be an alternative.

@jdebacker
Copy link
Member Author

@rickecon Version bump done. Will look into the alternative to PyGAM - thanks.

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.

3 participants