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

[query] Allow Graphite log and logarithm to accept float64 base arg #3145

Merged
merged 5 commits into from
Mar 17, 2021

Conversation

robskillington
Copy link
Collaborator

What this PR does / why we need it:

Previously log/logarithm only supported integer log base.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

@robskillington robskillington changed the title [query] Fix Graphite log and logarithm to accept float64 base arg [query] Allow Graphite log and logarithm to accept float64 base arg Jan 31, 2021
Copy link
Collaborator

@wesleyk wesleyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

})
testLogarithm(t, 3.142, func(output *ts.Series) {
xtest.Equalish(t, 0, output.ValueAt(1))
xtest.Equalish(t, 0.6054429879326457, output.ValueAt(2))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mind leaving a note how these values are derived?

Copy link
Collaborator

@arnikola arnikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@robskillington robskillington enabled auto-merge (squash) March 17, 2021 21:39
@robskillington robskillington merged commit 6b91769 into master Mar 17, 2021
@robskillington robskillington deleted the r/logarithm-support-float branch March 17, 2021 23:08
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