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

[SPARK-28858][ML][PYSPARK] add tree-based transformation in the py side #25566

Closed
wants to merge 1 commit into from

Conversation

zhengruifeng
Copy link
Contributor

What changes were proposed in this pull request?

expose the newly added tree-based transformation in the py side

Why are the changes needed?

function parity

Does this PR introduce any user-facing change?

yes, add setLeafCol & getLeafCol in the py side

How was this patch tested?

added tests & local tests

@zhengruifeng
Copy link
Contributor Author

This PR follows current pyspark's convention, which do support no setter/getter for model.

I am looking into the hierarchy of both scala and py side, and will open a ticket on the sync in next one or two weeks. I think we can sync them in a batch in the future, so in this PR I stick to pyspark's convention.

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I guess unfortunately all of the examples predict a leaf ID of 0?

@zhengruifeng
Copy link
Contributor Author

@srowen Yes. But I try other inputs and the returned leafId can be 1.

@zhengruifeng
Copy link
Contributor Author

should I change the input dataset to make some output != 0? @srowen

@srowen
Copy link
Member

srowen commented Aug 23, 2019

I wouldn't change the examples unless it's easy. It's not a big deal.

@SparkQA
Copy link

SparkQA commented Aug 23, 2019

Test build #109631 has finished for PR 25566 at commit edc8235.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@BryanCutler BryanCutler left a comment

Choose a reason for hiding this comment

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

LGTM

@BryanCutler
Copy link
Member

merged to master, thanks @zhengruifeng !

@zhengruifeng zhengruifeng deleted the py_tree_path branch August 26, 2019 02:10
@zhengruifeng
Copy link
Contributor Author

Thanks for reviewing! @BryanCutler @srowen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants