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

Periodic transform #2

Closed
glennmoy opened this issue Feb 1, 2021 · 6 comments · Fixed by #10
Closed

Periodic transform #2

glennmoy opened this issue Feb 1, 2021 · 6 comments · Fixed by #10
Assignees
Labels
new transform New transform request

Comments

@glennmoy
Copy link
Member

glennmoy commented Feb 1, 2021

A transform that applies a periodic function to the data with arguments

  • f - the periodic function to apply
  • period - the period of the function f
  • phase_shift - an optional phase shift to be applied to the function argument

Note that phase_shift is not strictly necessary for this function from a feature engineering perspective but may be required for debugging purposes and/or comparison against our current use of this function.

MWE

x = 1:1000

# period/phase could also just be positional args
periodic = Periodic(cos, period=5, phase_shift=2)

Transforms.apply!(x, periodic)
@glennmoy glennmoy added the new transform New transform request label Feb 1, 2021
@bencottier
Copy link
Contributor

Note that phase_shift is not strictly necessary for this function from a feature engineering perspective but may be required for debugging purposes and/or comparison against our current use of this function.

I don't quite understand, do you mean that the phase shift could be a separate transform?

@bencottier bencottier self-assigned this Feb 1, 2021
@bencottier
Copy link
Contributor

Any preference for 2π inside or outside period? E.g. sin((2π / P.period) .* x) vs. sin((1 / P.period) .* x)?

Does this depend on whether the periodic function is sinusoidal or something else e.g. triangular wave?

@bencottier
Copy link
Contributor

Not sure if this should be explicitly time-based. Should period::Dates.Period, period::Real, or either?

@glennmoy
Copy link
Member Author

glennmoy commented Feb 1, 2021

Not sure if this should be explicitly time-based. Should period::Dates.Period, period::Real, or either?

No reason why it should be restricted to just TimeTypes IMO - it might help to keep it flexible.

@glennmoy
Copy link
Member Author

glennmoy commented Feb 1, 2021

Not sure if this should be explicitly time-based. Should period::Dates.Period, period::Real, or either?

I would say inside - check out our current code in FeatureEngineering for reference.

@glennmoy
Copy link
Member Author

glennmoy commented Feb 1, 2021

Note that phase_shift is not strictly necessary for this function from a feature engineering perspective but may be required for debugging purposes and/or comparison against our current use of this function.

I don't quite understand, do you mean that the phase shift could be a separate transform?

Check out the first issues in our FeatureEngineering package for more context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new transform New transform request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants