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

Transformations: Use the display name of the original y field for the predicted field of the regression analysis transformation. #81332

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

oscarkilhed
Copy link
Contributor

What is this feature?

This sets a better name for the field with the predicted value from the regression analysis transformation

Why do we need this feature?

See #81317

Who is this feature for?

Users of the regression analysis transformation.

Which issue(s) does this PR fix?:

Fixes #81317

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.

Copy link
Contributor

@baldm0mma baldm0mma left a comment

Choose a reason for hiding this comment

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

Have you checked if this denigrates performance at all? I know this specific function (getFieldDisplayName) has created some recent problems for us, most specifically dealt with in this PR @leeoniya has up. 🤔 I don't think this should necessarily stop this change, but just something to keep in mind.

Copy link
Contributor

@baldm0mma baldm0mma left a comment

Choose a reason for hiding this comment

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

This change looks good to me, bearing in mind my previous comment, and maybe waiting for the getFieldDisplayName work to be merged first? 🤔

Copy link
Contributor

@nmarrs nmarrs left a comment

Choose a reason for hiding this comment

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

LGTM - re: current performance issues with getFieldDisplayName I don't think that is a blocker on getting this work merged :)

Copy link
Collaborator

@codeincarnate codeincarnate left a comment

Choose a reason for hiding this comment

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

Looks good to me as well! I agree that I think this would be fine on the performance front, but may be worth keeping an eye on it 😄

@oscarkilhed oscarkilhed merged commit 9a4534a into main Jan 29, 2024
29 checks passed
@oscarkilhed oscarkilhed deleted the oscark/use-displayname-for-regression-fieldname branch January 29, 2024 09:22
Ukochka pushed a commit that referenced this pull request Feb 14, 2024
… predicted field of the regression analysis transformation. (#81332)

Fix name of regression transformation field
@aangelisc aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression transformation not using Y field name
5 participants