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-22735][ML][DOC] Added VectorSizeHint docs and examples. #20285

Closed
wants to merge 9 commits into from

Conversation

MrBago
Copy link
Contributor

@MrBago MrBago commented Jan 17, 2018

What changes were proposed in this pull request?

Added documentation for new transformer.

@SparkQA
Copy link

SparkQA commented Jan 17, 2018

Test build #86223 has finished for PR 20285 at commit b4f2c71.

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

@SparkQA
Copy link

SparkQA commented Jan 17, 2018

Test build #86221 has finished for PR 20285 at commit 85d0db0.

  • 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.

Just some minor things, but looks pretty good, could you post a screenshot of the doc after building? Are you planning on adding a Java example?

@@ -1283,6 +1283,48 @@ for more details on the API.
</div>
</div>

## VectorSizeHint

It can sometimes be useful to explicitly specify the size of the vectors an a column of
Copy link
Member

Choose a reason for hiding this comment

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

typo 'an a column' -> 'in a column'

meatadata.

`VectorSizeHint` can also take an optional `handleInvalid` parameter which controls its
behaviour when the vector column contains nulls for vectors of the wrong size. By default
Copy link
Member

Choose a reason for hiding this comment

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

typo: 'nulls for vectors..' -> 'nulls or vectors'

if __name__ == "__main__":
spark = SparkSession\
.builder\
.appName("VectorAssemblerExample")\
Copy link
Member

Choose a reason for hiding this comment

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

should be "VectorSizeHintExample" - same with other apis


sizeHint = VectorSizeHint(
inputCol="userFeatures",
handleInvalid="sip",
Copy link
Member

Choose a reason for hiding this comment

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

typo "sip" -> "skip"

@SparkQA
Copy link

SparkQA commented Jan 17, 2018

Test build #86287 has finished for PR 20285 at commit c0a53de.

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

@MrBago
Copy link
Contributor Author

MrBago commented Jan 18, 2018

screen shot 2018-01-17 at 4 19 40 pm

@MrBago
Copy link
Contributor Author

MrBago commented Jan 18, 2018

screen shot 2018-01-17 at 4 20 02 pm

@MrBago
Copy link
Contributor Author

MrBago commented Jan 18, 2018

Thanks for the review @BryanCutler, I've added a java example & uploaded 2 screenshots.

@SparkQA
Copy link

SparkQA commented Jan 18, 2018

Test build #86302 has finished for PR 20285 at commit 0cdfc1b.

  • 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.

thanks, LGTM!

import static org.apache.spark.sql.types.DataTypes.*;

// $example on$
// $example off$
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the above two lines?

inputCols=["hour", "mobile", "userFeatures"],
outputCol="features")

# This dataframe can be used by used by downstream transformers as before
Copy link
Member

Choose a reason for hiding this comment

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

I think there is some typos here.

.setInputCols(Array("hour", "mobile", "userFeatures"))
.setOutputCol("features")

// This dataframe can be used by used by downstream transformers as before
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

@@ -1283,6 +1283,56 @@ for more details on the API.
</div>
</div>

## VectorSizeHint

It can sometimes be useful to explicitly specify the size of the vectors a column of
Copy link
Member

Choose a reason for hiding this comment

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

a column of Vector -> for a column of Vector?

`VectorType`. For example, `VectorAssembler` uses size information from its input columns to
produce size information and metadata for its output column. While in some cases this information
can be obtained by inspecting the contents of the column, in a streaming dataframe the contents are
not available until the stream is started. `VectorSizeHint` allows a user to explicitly specify the
Copy link
Member

Choose a reason for hiding this comment

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

nit: a user -> an user

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 don't know if the spark style guide covers this, but I believe "a user" is generally the prefered form, https://english.stackexchange.com/a/105117.

vector size for a column so that `VectorAssembler`, or other transformers that might
need to know vector size, can use that column as an input.

To use `VectorSizeHint` a user must set the `inputCol` and `size` parameters. Applying this
Copy link
Member

Choose a reason for hiding this comment

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

a user -> an user

Copy link
Contributor

Choose a reason for hiding this comment

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

a user is correct because users's pronunciation starts with y

@SparkQA
Copy link

SparkQA commented Jan 19, 2018

Test build #86366 has finished for PR 20285 at commit 6228902.

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

@MrBago
Copy link
Contributor Author

MrBago commented Jan 22, 2018

I'd like to prioritize getting this merged to ensure our documentation is complete for the 2.3 release. @viirya and @WeichenXu123 would you mind having another look at it?

createStructField("userFeatures", new VectorUDT(), false),
createStructField("clicked", DoubleType, false)
});
Row row = RowFactory.create(0, 18, 1.0, Vectors.dense(0.0, 10.0, 0.5), 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.

Hi, @MrBago . It seems that we need to add one more row here.

RowFactory.create(0, 18, 1.0, Vectors.dense(0.0, 10.0), 0.0);

Copy link
Contributor

@WeichenXu123 WeichenXu123 left a comment

Choose a reason for hiding this comment

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

LGTM except a minor comment. 👍

`handleInvalid` is set to "error", indicating an exception should be thrown. This parameter can
also be set to "skip", indicating that rows containing invalid values should be filtered out from
the resulting dataframe, or `optimistic` indicating that all rows should be kept. When
`handleInvalid` is set to `optimistic` the user takes responsibility for ensuring that the column
Copy link
Contributor

Choose a reason for hiding this comment

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

optimistic --> "optimistic"
the backquote only used on code vars.

createStructField("clicked", DoubleType, false)
});
Row row0 = RowFactory.create(0, 18, 1.0, Vectors.dense(0.0, 10.0, 0.5), 1.0);
Row row1 = RowFactory.create(0, 18, 1.0, Vectors.dense(0.0, 10.0, 0.5), 1.0);
Copy link
Member

@dongjoon-hyun dongjoon-hyun Jan 23, 2018

Choose a reason for hiding this comment

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

Can we use the same data set with the other code?
I mean the second row is different from what I suggested and the other examples'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, yes should be fixed now.

@SparkQA
Copy link

SparkQA commented Jan 23, 2018

Test build #86541 has finished for PR 20285 at commit 4de3f81.

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

@SparkQA
Copy link

SparkQA commented Jan 23, 2018

Test build #86543 has finished for PR 20285 at commit 6055a8c.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

Copy link
Contributor

@WeichenXu123 WeichenXu123 left a comment

Choose a reason for hiding this comment

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

LGTM.

`handleInvalid` is set to "error", indicating an exception should be thrown. This parameter can
also be set to "skip", indicating that rows containing invalid values should be filtered out from
the resulting dataframe, or "optimistic" indicating that all rows should be kept. When
`handleInvalid` is set to "optimistic" the user takes responsibility for ensuring that the column
Copy link
Contributor

@mengxr mengxr Jan 23, 2018

Choose a reason for hiding this comment

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

Not clear to me what is the expected behaivor of optimistic. How is it different from error? Does it output null?

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've updated it, let me know if you think we can still make it more clear.

<div class="codetabs">
<div data-lang="scala" markdown="1">

Refer to the [VectorSizeHint Scala docs](api/scala/index.html#org.apache.spark.ml.feature.VectorSizeHint)
Copy link
Contributor

@mengxr mengxr Jan 23, 2018

Choose a reason for hiding this comment

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

minor: Do we need to mention Scala explicitly here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

screen shot 2018-01-23 at 1 54 28 pm

I don't think so :), but I think we should leave it to be consistent with other examples.

@mengxr
Copy link
Contributor

mengxr commented Jan 23, 2018

LGTM. Merged into master and branch-2.3. Thanks!

asfgit pushed a commit that referenced this pull request Jan 23, 2018
## What changes were proposed in this pull request?

Added documentation for new transformer.

Author: Bago Amirbekian <[email protected]>

Closes #20285 from MrBago/sizeHintDocs.

(cherry picked from commit 05839d1)
Signed-off-by: Xiangrui Meng <[email protected]>
@SparkQA
Copy link

SparkQA commented Jan 23, 2018

Test build #86548 has finished for PR 20285 at commit 3055eec.

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

@asfgit asfgit closed this in 05839d1 Jan 23, 2018
@dongjoon-hyun
Copy link
Member

Hi, @mengxr .
Could you resolve the JIRA, too?

Thanks!

@MrBago MrBago deleted the sizeHintDocs branch January 24, 2018 19:27
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.

7 participants