Skip to content

Conversation

@sharp-pixel
Copy link
Contributor

What changes were proposed in this pull request?

The line SchemaUtils.appendColumn(schema, $(predictionCol), IntegerType) did not modify the variable schema, hence only the last line had any effect. A temporary variable is used to correctly append the two columns predictionCol and probabilityCol.

How was this patch tested?

Manually.

Please review http://spark.apache.org/contributing.html before opening a pull request.

@srowen
Copy link
Member

srowen commented Aug 18, 2017

Yeah I think you're right. There's a similar issue in AFTSurvivalRegressionParams
CC @yanboliang

@yanboliang
Copy link
Contributor

+1 @srowen, this is a bug.
@sharp-pixel Would you mind to fix both GaussianMixture and AFTSurvivalRegression? It's better to file a JIRA firstly and add some unit tests. Thanks.

@sharp-pixel
Copy link
Contributor Author

I just fixed AFTSurvivalRegression also.

SchemaUtils.checkColumnType(schema, $(featuresCol), new VectorUDT)
SchemaUtils.appendColumn(schema, $(predictionCol), IntegerType)
SchemaUtils.appendColumn(schema, $(probabilityCol), new VectorUDT)
val schema_temp = SchemaUtils.appendColumn(schema, $(predictionCol), IntegerType)
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit: Java/Scala use camelCase var names. Maybe call this withPredictionCol or something, as it's more descriptive anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad! Just committed with more descriptive variables in camelCase. Thanks

@srowen
Copy link
Member

srowen commented Aug 20, 2017

I started tests. One last request, start the title with [MINOR]. See https://spark.apache.org/contributing.html

@SparkQA
Copy link

SparkQA commented Aug 20, 2017

Test build #3892 has finished for PR 18980 at commit d14c21e.

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

@sharp-pixel sharp-pixel changed the title Correct validateAndTransformSchema in GaussianMixture [MINOR]Correct validateAndTransformSchema in GaussianMixture Aug 20, 2017
@sharp-pixel sharp-pixel changed the title [MINOR]Correct validateAndTransformSchema in GaussianMixture [MINOR]Correct validateAndTransformSchema in GaussianMixture and AFTSurvivalRegression Aug 20, 2017
@srowen
Copy link
Member

srowen commented Aug 20, 2017

Actually, on second thought, we should make a JIRA for this and link it. It's a non-trivial bug fix. I can do that if you're busy, but feel free to. I'll merge it soon anyway

@sharp-pixel
Copy link
Contributor Author

As I am fairly new to JIRA, feel free to do that if you can. Thanks!

asfgit pushed a commit that referenced this pull request Aug 20, 2017
…SurvivalRegression

## What changes were proposed in this pull request?

The line SchemaUtils.appendColumn(schema, $(predictionCol), IntegerType) did not modify the variable schema, hence only the last line had any effect. A temporary variable is used to correctly append the two columns predictionCol and probabilityCol.

## How was this patch tested?

Manually.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: Cédric Pelvet <[email protected]>

Closes #18980 from sharp-pixel/master.

(cherry picked from commit 73e04ec)
Signed-off-by: Sean Owen <[email protected]>
asfgit pushed a commit that referenced this pull request Aug 20, 2017
…SurvivalRegression

## What changes were proposed in this pull request?

The line SchemaUtils.appendColumn(schema, $(predictionCol), IntegerType) did not modify the variable schema, hence only the last line had any effect. A temporary variable is used to correctly append the two columns predictionCol and probabilityCol.

## How was this patch tested?

Manually.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: Cédric Pelvet <[email protected]>

Closes #18980 from sharp-pixel/master.

(cherry picked from commit 73e04ec)
Signed-off-by: Sean Owen <[email protected]>
@asfgit asfgit closed this in 73e04ec Aug 20, 2017
@srowen
Copy link
Member

srowen commented Aug 20, 2017

Merged to master/2.2/2.1

MatthewRBruce pushed a commit to Shopify/spark that referenced this pull request Jul 31, 2018
…SurvivalRegression

## What changes were proposed in this pull request?

The line SchemaUtils.appendColumn(schema, $(predictionCol), IntegerType) did not modify the variable schema, hence only the last line had any effect. A temporary variable is used to correctly append the two columns predictionCol and probabilityCol.

## How was this patch tested?

Manually.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: Cédric Pelvet <[email protected]>

Closes apache#18980 from sharp-pixel/master.

(cherry picked from commit 73e04ec)
Signed-off-by: Sean Owen <[email protected]>
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