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.

Sign up for GitHub

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

Jump to bottom

[SPARK-15938]Adding "support" property to MLlib Association Rule #13656

Closed
wants to merge 4 commits into from
Closed

[SPARK-15938]Adding "support" property to MLlib Association Rule #13656

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60efd05
add support for association rule
hhbyyh Jun 14, 2016
f638d25
Merge remote-tracking branch 'upstream/master' into supportAsso
hhbyyh Jun 23, 2016
8b16676
add data size
hhbyyh Jun 23, 2016
ed384c7
java style
hhbyyh Jun 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions mllib/src/main/scala/org/apache/spark/mllib/fpm/AssociationRules.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,13 @@ object AssociationRules {
@Since("1.5.0")
def confidence: Double = freqUnion.toDouble / freqAntecedent

/**
* Returns the support of the rule. Current implementation would return the number of
* co-occurrence of antecedent and consequent.
*/
@Since("2.1.0")
def support: Double = freqUnion.toDouble

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentionally typed as Double. In the future, it could be fraction value ( < 1.0).

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 the meaning of this should ever be overloaded. Support is a count.

Copy link
Contributor Author

@hhbyyh hhbyyh Jun 14, 2016 •
edited

Choose a reason for hiding this comment

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

Two major considerations:

  1. In most definition and text books, support is a fraction value in [0.0, 1.0]. It's possible for us to align with it in the future.
  2. Current implementation of Association rule actually allows both
    freqUnion: Double, and
    freqAntecedent: Double
    to be fraction value [0.0, 1.0] although they are both counts now. I don't want to destroy the flexibility and break API in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, we use support as a fraction. Well, then best to be consistent and return it as a fraction of the data set size. I can't imagine having a method sometimes return a value with one type of semantics and sometimes another. Just make two methods.

freqUnion however appears to be a count only, and is even explicitly called a 'frequency'.

Copy link
Contributor Author

@hhbyyh hhbyyh Jun 14, 2016 •
edited

Choose a reason for hiding this comment

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

I suppose freqUnion is made as a Double on purpose for the same reason. (flexibility for the future)

Making support a fraction now requires that we must keep the dataset size info in FPGrowthModel and AssociationRule. Yet that would introduce API change in the constructor. I thought we should avoid breaking API between 2.0 and 2.1.

Copy link
Member

Choose a reason for hiding this comment

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

Dunno, that seems like a mistake to me. It should be a Long if it's a count, and should expose alternative factory methods to accept input of different types if needed. Overloading one argument seems like a hack and I'd prefer not to extend it (or fix it).

See SPARK-15930 which concerns adding the input size just for this reason, I assume. We haven't released 2.0, and so could in theory still put in a change to the constructor. I agree, we might however have to deprecate the existing one, add a new one, and still deal with calls to the old constructor, which would mean it's not possible to compute values that are a fraction of the whole data set. This in turn may argue for clearly separating inputs/outputs that are counts vs percentages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find it hard to just deprecating the old constructor and still keep support as a fraction if no dataset size is passed in.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's not possible to implement in that case. There's an argument for just adding the parameter and removing the old constructor for 2.0 in order to support this without the convolutions. I'd love to get a thumbs up from @jkbradley or @mengxr though.

Copy link
Contributor

Choose a reason for hiding this comment

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

support should be a fraction to be consistent with the semantic of minSupport in FPGrowth and PrefixSpan. There should be a compatible way to add support. Rule is not a case class and its constructor is package private. So this should be easy to add. Another approach is to add total number of records in the model, so people can calculate the support easily.

require(antecedent.toSet.intersect(consequent.toSet).isEmpty, {
val sharedItems = antecedent.toSet.intersect(consequent.toSet)
s"A valid association rule must have disjoint antecedent and " +
Expand Down