imatiach-msft commented on issue #21632: [SPARK-19591][ML][MLlib] Add sample weights to decision trees URL: https://github.com/apache/spark/pull/21632#issuecomment-456670961 "up to your judgment on whether to add a new overload to DecisionTreeMetadata to simplify the test code" This is a tough decision to make; I would prefer not to modify the source code for the sake of tests, but modifying a lot of test code to call the DecisionTreeMetadata.buildMetadata with LabeledPoint converted to instances instead of LabeledPoint is bad too. There are other options as well. I could make DecisionTreeMetadata.buildMetadata accept an RDD[_] and then dynamically figure out the type but this doesn't seem like a good choice either. I could also create a wrapper around buildMetadata in the test code and then call that wrapper from all tests which should make maintaining code easier in the future (eg the conversion could have been done in the wrapper) but that would only introduce more changes - not less - to the PR, and it also creates another level of indirection which may make the test code more confusing. The current code seems the slightly better choice of the four options listed above (and there may be other options as well), but if there is a strong preference toward one of the other choices I would be glad to update the PR.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
