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

MQE: Add support for deriv function #9858

Merged
merged 9 commits into from
Nov 13, 2024
Merged

MQE: Add support for deriv function #9858

merged 9 commits into from
Nov 13, 2024

Conversation

lamida
Copy link
Contributor

@lamida lamida commented Nov 10, 2024

What this PR does

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

  • Tests updated.
  • n.a Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • n.a about-versioning.md updated with experimental features.

@lamida lamida marked this pull request as ready for review November 10, 2024 18:14
@lamida lamida requested a review from a team as a code owner November 10, 2024 18:14
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add tests that include:

  • native histograms
  • special cases like the range being all 0, the range all containing the same non-zero value, the range containing NaN, Inf, and -Inf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I will add today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test in 185ea64

@lamida lamida force-pushed the lamida/mqe-deriv-function branch 3 times, most recently from 185ea64 to 77641d8 Compare November 12, 2024 18:34
@lamida lamida requested a review from charleskorn November 12, 2024 19:10
Copy link
Contributor

@charleskorn charleskorn left a comment

Choose a reason for hiding this comment

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

LGTM modulo nit

@@ -460,3 +460,38 @@ load 6m

eval range from 0 to 96m step 6m round(series, scalar(toNearest))
{a="b"} -5 2.7 0.5 20 10 3.25 0 1000.01 0 -1000000 NaN NaN NaN NaN _ _ 10

# Testing deriv
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Similar comment to #9859 (comment) applies here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored the test in 6fdb1f6

Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
@lamida lamida force-pushed the lamida/mqe-deriv-function branch from 77641d8 to 7933bf1 Compare November 13, 2024 03:46
Signed-off-by: Jon Kartago Lamida <[email protected]>
@lamida lamida merged commit 98b43bf into main Nov 13, 2024
29 checks passed
@lamida lamida deleted the lamida/mqe-deriv-function branch November 13, 2024 08:57
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.

2 participants