Skip to content

[SPARK-28243][PYSPARK][ML][FOLLOW-UP] Move Python DecisionTreeParams to regression.py#25406

Closed
huaxingao wants to merge 1 commit intoapache:masterfrom
huaxingao:spark-28243
Closed

[SPARK-28243][PYSPARK][ML][FOLLOW-UP] Move Python DecisionTreeParams to regression.py#25406
huaxingao wants to merge 1 commit intoapache:masterfrom
huaxingao:spark-28243

Conversation

@huaxingao
Copy link
Contributor

What changes were proposed in this pull request?

Leave shared.py untouched. Move Python DecisionTreeParams to regression.py

How was this patch tested?

Use existing tests

@huaxingao
Copy link
Contributor Author

I move DecisionTreeParams to regression.py for now. May need to move again after we re-org the python code.

@SparkQA
Copy link

SparkQA commented Aug 11, 2019

Test build #108927 has finished for PR 25406 at commit 45b9575.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class DecisionTreeParams(Params):

@srowen
Copy link
Member

srowen commented Aug 11, 2019

Maybe @mengxr or @brkyvz have a thought on this, having originally set up this mechanism. Is it reasonable to move to just defining these in the normal class hierarchy?

@huaxingao
Copy link
Contributor Author

@zhengruifeng

@zhengruifeng
Copy link
Contributor

@huaxingao Do you update the shared.py by run the cmd?

@huaxingao
Copy link
Contributor Author

Yes, shared.py was generated by running the cmd. @zhengruifeng

Copy link
Contributor

@zhengruifeng zhengruifeng left a comment

Choose a reason for hiding this comment

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

I am Ok with this modification, however the DecisionTreeParams in the py side is still not sync with the scala side. We can leave this confilict alone, and make them in sync in the future.

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.

Eh, yeah this is my fault for not realizing this is how the parameters are defined in some cases. I imagine there are other sync problems. It'd be great to re-sync them, though I do wonder whether this mechanism is worth it. That can happen later if you're willing. I think this is OK.

@srowen
Copy link
Member

srowen commented Aug 15, 2019

Merged to master

@srowen srowen closed this in ba5ee27 Aug 15, 2019
@huaxingao
Copy link
Contributor Author

Thanks @srowen @zhengruifeng
I will be more careful next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants