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

issue:fix for issue #690 - incorrect computation of n #708

Merged
merged 4 commits into from
Dec 19, 2023

Conversation

StatKumar
Copy link
Contributor

This is the fix for the issue # 690.

Math error was occurring when the len(n) = 1. While slicing the array, the last value of the array is not considered, and due to slicing, a variable's value was set to 0, and later in the code, log of that variable was taken, which led to math error.

Fix:
n = np.sum(~missing[firstnonmiss:lastnonmiss])
changed to
n = np.sum(~missing[firstnonmiss:])

@CLAassistant
Copy link

CLAassistant commented Nov 24, 2023

CLA assistant check
All committers have signed the CLA.

@jmoralez
Copy link
Member

Hey @StatKumar. Can you please:

  • Read the contributing guidelines and make the change in the notebook.
  • Make the change I suggested here (lastnonmiss = missing_idxs.max() + 1). The proposed change here would unnecessarily break the code for people who are directly using the AutoARIMA model and passing arrays like [nan, 1, 2, 3, nan].
  • Also change the other places where this is happening, which I referenced here as well.

@StatKumar
Copy link
Contributor Author

@jmoralez Thank you for the guidelines. Will update soon.

@StatKumar StatKumar closed this Nov 24, 2023
@jmoralez
Copy link
Member

Can you update this PR instead of opening a new one?

@StatKumar StatKumar reopened this Nov 25, 2023
@StatKumar
Copy link
Contributor Author

Sure @jmoralez

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@StatKumar
Copy link
Contributor Author

@jmoralez Updated the pull request with a new commit. Please let me know if anything else needs to be done. Thank you for taking time in guiding me through my first issue in statsforecast.

@jmoralez
Copy link
Member

Thanks @StatKumar! This also happens in the myarima function. Can you add the fix there as well? Also please add a cell at the bottom with the original example from #690 (AutoARIMA().fit(np.array([1.0]))) to make sure that the fix works.

@jmoralez jmoralez linked an issue Dec 19, 2023 that may be closed by this pull request
@jmoralez jmoralez merged commit e29838d into Nixtla:main Dec 19, 2023
17 checks passed
@jmoralez jmoralez added the fix label Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ARIMA] Incorrect computation of n
3 participants