Skip to content

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Jan 23, 2025

What this PR does

This PR adds support in MQE for predict_linear.

Which issue(s) this PR fixes or relates to

Part of #10067

Checklist

  • Tests updated.
  • [n/a] Documentation added.
  • [covered by Mimir Query Engine #10067] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • [n/a] about-versioning.md updated with experimental features.

@charleskorn charleskorn force-pushed the charleskorn/mqe-predict-linear branch from a553906 to 8893f16 Compare January 23, 2025 10:50
@charleskorn charleskorn marked this pull request as ready for review January 23, 2025 12:05
@charleskorn charleskorn requested a review from a team as a code owner January 23, 2025 12:05
Copy link
Contributor

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

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

We should perhaps pass this through the gantlet.

fHead, fTail := step.Floats.UnsafePoints()

if len(fHead)+len(fTail) == 1 && step.Histograms.Any() {
if step.Floats.Any() && step.Histograms.Any() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should address the annotations for deriv in a separate PR given the functionality change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow sorry - which functionality are you referring to?

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR changes the pre-existing deriv function in how it handles annotations. It might be easier to review if we do that separately first as its own PR. That said, I'm okay doing it here if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the changes are minor enough that a separate PR is not necessary.

@charleskorn
Copy link
Contributor Author

We should perhaps pass this through the gantlet.

Added in 63f59a7.

@charleskorn
Copy link
Contributor Author

CI is failing due to the changes in #10533, I'll fix this after #10553 is merged.

@charleskorn charleskorn force-pushed the charleskorn/mqe-predict-linear branch from 63f59a7 to ce747ee Compare February 3, 2025 02:37
@charleskorn
Copy link
Contributor Author

CI is failing due to the changes in #10533, I'll fix this after #10553 is merged.

This should be resolved now.

@charleskorn charleskorn force-pushed the charleskorn/mqe-predict-linear branch from ce747ee to 328d798 Compare February 3, 2025 23:06
@charleskorn charleskorn enabled auto-merge (squash) February 3, 2025 23:24
@charleskorn charleskorn merged commit e3862e4 into main Feb 3, 2025
28 checks passed
@charleskorn charleskorn deleted the charleskorn/mqe-predict-linear branch February 3, 2025 23:45
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