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

[ML] Rename data frame analytics maximum_number_trees to max_trees #53300

Conversation

dimitris-athanasiou
Copy link
Contributor

Deprecates maximum_number_trees parameter of classification and
regression and replaces it with max_trees.

Deprecates `maximum_number_trees` parameter of classification and
regression and replaces it with `max_trees`.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

Left a question, everything else looks good

@@ -43,6 +43,9 @@
"maximum_number_trees" : {
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the old mapping can't you? If a new index is being created we don't need it and if the index is an old one then the mapping is already present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense to keep it around for the possibility that a job using the old field name is cloned into a new index.

Copy link
Member

Choose a reason for hiding this comment

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

Won't the cloned job go through the process of renaming the field automatically as it is parsed and written out to xcontent.

I think what is bothering me is that we can't comment on the mapping json // remove this at 9.0.0 and no doubt it will be forgotten about. If it can go now I'd rather we get rid of it.

Can you at least try removing the old mapping and see if the upgrade tests pass please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. There is a test failing but I can fix the test. I'll give it a go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dimitris-athanasiou
Copy link
Contributor Author

@elasticmachine update branch

@dimitris-athanasiou dimitris-athanasiou force-pushed the rename-df-analytics-maximum-number-trees-to-max-trees branch from 8584c94 to e46e008 Compare March 10, 2020 13:39
@dimitris-athanasiou
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM

@dimitris-athanasiou dimitris-athanasiou merged commit 5a32f50 into elastic:master Mar 11, 2020
@dimitris-athanasiou dimitris-athanasiou deleted the rename-df-analytics-maximum-number-trees-to-max-trees branch March 11, 2020 08:33
dimitris-athanasiou added a commit that referenced this pull request Mar 11, 2020
…es (#53300) (#53390)

Deprecates `maximum_number_trees` parameter of classification and
regression and replaces it with `max_trees`.

Backport of #53300
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.

4 participants