Skip to content

Conversation

@ogirardot
Copy link

The default method will use Guava's Ordering instead of
java.util.Comparator.naturalOrder() because it's not available
in Java 7, only in Java 8.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this and the project doesn't depend directly on commons-collections anyway IIRC.

@ogirardot
Copy link
Author

I removed the bad import - it was a typo

@ogirardot
Copy link
Author

I've taken your reviews into account.

@srowen
Copy link
Member

srowen commented Apr 18, 2015

ok to test

@SparkQA
Copy link

SparkQA commented Apr 18, 2015

Test build #30517 has finished for PR 5571 at commit 7c5f4ab.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Params(
    • sealed abstract class Node extends Serializable
    • sealed trait Split extends Serializable
    • final class CategoricalSplit(
    • final class ContinuousSplit(override val featureIndex: Int, val threshold: Double) extends Split
    • trait DecisionTreeModel
  • This patch does not change any dependencies.

@ogirardot
Copy link
Author

Seems like the precision for float is needed in the test.
I'll add it back

@srowen
Copy link
Member

srowen commented Apr 18, 2015

No, just assert that == is true. Looks like the framework actually rejects use of this method? It didn't say it didn't equal, weirdly

The default method will use Guava's Ordering instead of
java.util.Comparator.naturalOrder() because it's not available
in Java 7, only in Java 8.
@ogirardot
Copy link
Author

I pushed the modifications,
The third time is always the winning one ! :)

@SparkQA
Copy link

SparkQA commented Apr 18, 2015

Test build #30519 has finished for PR 5571 at commit 99c3416.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@SparkQA
Copy link

SparkQA commented Apr 18, 2015

Test build #30521 has finished for PR 5571 at commit 7fe2e9e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@srowen
Copy link
Member

srowen commented Apr 18, 2015

LGTM. Though I imagine this is not controversial, I would love to get a nod from @JoshRosen or @pwendell or @rxin as it offers a new API method.

@ogirardot
Copy link
Author

of course, have a nice weekend :)

@rxin
Copy link
Contributor

rxin commented Apr 19, 2015

LGTM. Merging in master.

@asfgit asfgit closed this in 8fbd45c Apr 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants