-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 Cosine, ACF and PACF transformers #509
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Afzal-Ind - thanks for submitting the PR! I had a quick look, the overall structure looks good, I left two comments in the code.
Next thing to do is to write unit tests for the transformers you added. For an example, check out the unit tests for the boxcox transformer
There were some merge conflicts, but I resolved them with the latest commit. To avoid them, you need to make sure to sync your fork before you check out a new branch, |
Hi @mloning. Thanks a lot... I will take care of this. |
Hi @Afzal-Ind looks much better now - next step is to write some simple unit tests as described above! |
…/transformers/sktime/transformer/series/cos.py
…/transformers/series/cos.py
…e/transformers/series/acf.py
Hi @Afzal-Ind - okay renaming solved one issue. Now the issue seems to be related to how multivariate data is handled. I'd start by setting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd start by setting
check_series(..., enforce_univariate=True)
to get this to run first, we can then add support for multivariate once everything is working.
Fine now. I can start trying this way. Thanks.
Hi @Afzal-Ind if you have time to work on this, we could try to include it in the next release which I'm currently working on. For now, we can just remove the |
Fine @mloning . I am looking forward to working with u on 'inverse_transformer' in next release. Currently dealing with AutoAR. Thanks. |
@Afzal-Ind I want to push the next release in the next few days, so if you have time to finish it, that would be great. Otherwise we'll include in the next one after that. Of course let me know if you get stuck and I can help! |
I was kind of busy for two days. I would start my work on that from tomorrow and let u update. |
Hi @Afzal-Ind, I've taken a look and made some changes - hopefully all tests pass now. Take a look at my commit to understand what I changed - happy to discuss this if you have any questions! |
Reference Issues/PRs
Fixes #483
What does this implement/fix? Explain your changes.
Added two transformations.
Does your contribution introduce a new dependency? If yes, which one?
No
PR checklist