Update time_conversion.py #3
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add adjustments in handle_endpoint_differences to resolve issues related to quarterly data
|
This looks really great! All the old tests are passing, so it should be good to merge. Can you just add a regression test? Basically add |
Add test files
|
No need, you can keep working in this PR. You can check this test for what I'm currently doing. The file you uploaded is the "ground truth" from the R function? |
|
Yes, I did run it in R on my side. Here is the code for your reference: Is this match what you do to generate "ground truth" in R as well? |
|
Ok so I double check the results on my side from both Python and R, and found two completely different chow-lin regression results. Not sure where is the problem. But I post it here so that you, and my teammate who is also working on this, can take a look: Results from R: Call: Python code used to generate the result: Results from Python: |
|
The scale difference in the intercept suggests that either the optimizer isn't converging, or the data isn't the same. I'll have some time to take a look later today and I'll see if I can help get to the bottom of this. |
|
I foolishly didn't add a flag to return the raw optimizer output for debugging, I would suggest doing that as well and checking if it's converging. You can also try using |
|
Double checked on my side by comparing two dataframe together. The data used in the python side is the same as the R side! |
|
Gross. Can you confirm that the feature matrix we build and ultimate use on the RHS of the regression matches R? |
|
Just checked. Both matrix matches with each other! |
|
So the problem is either in the covariance matrix or the loss function. The covariance matrix should be easier to test, so I would start there. Can you check that for given rho, sigma, and n, |
|
I only generated the chow-lin covariance from the python side as I am unable to obtain the one through the source code in R(looks unaccessible on my side). I've add that file as tests/data/chowlin_covariance.csv. |
|
I believe I fixed the bug. I misunderstood this line in the paper:
For some reason I took this to mean that we don't minimize beta as part of this objective function, and instead compute it using the values of rho and sigma obtained from the minimization. Looking back I have no idea how I reached this conclusion. But now it's fixed, and the outputs match R. |
jessegrabowski
approved these changes
Aug 8, 2024
|
Thanks again for finding this, and for all your work debugging! |
|
No problem! Hopefully everything will work well after release! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add adjustments in handle_endpoint_differences to resolve issues related to quarterly data:
Setting C_mask as "None" and immediately return it so that "C_mask = C_mask or np.full(nl, True)" in ts_disagg.py/build_conversion_matrix will do "np.full(nl, True)" to make the matrix size match up.
Note: Not sure it is absolutely correct but it works!