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-10592] [ML] [PySpark] Deprecate weights and use coefficients instead in ML models #9311

Closed
wants to merge 4 commits into from
Closed

[SPARK-10592] [ML] [PySpark] Deprecate weights and use coefficients instead in ML models #9311

wants to merge 4 commits into from

Conversation

vectorijk
Copy link
Contributor

Deprecated in LogisticRegression and LinearRegression

@SparkQA
Copy link

SparkQA commented Oct 27, 2015

Test build #44470 has finished for PR 9311 at commit 4403baf.

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

@mengxr
Copy link
Contributor

mengxr commented Oct 27, 2015

You can trace all weights calls in an IDE. For example, I found this: src/spark/mllib/src/main/scala/org/apache/spark/ml/r/SparkRWrappers.scala:54.

@vectorijk
Copy link
Contributor Author

Thanks! It seems like I missed some. I will trace and keep on deprecation.

@vectorijk vectorijk changed the title [SPARK-10592] [ML] [PySpark] Deprecate weights and use coefficients instead in ML models [SPARK-10592] [WIP] [ML] [PySpark] Deprecate weights and use coefficients instead in ML models Oct 27, 2015
@mengxr
Copy link
Contributor

mengxr commented Oct 29, 2015

test this please

@SparkQA
Copy link

SparkQA commented Oct 29, 2015

Test build #44608 has finished for PR 9311 at commit 804c650.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

vectorijk
vectorijk reviewed Oct 30, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/regression/IsotonicRegression.scala
@@ -87,8 +87,8 @@ private[regression] trait IsotonicRegressionBase extends Params with HasFeatures
lit(1.0)
}
dataset.select(col($(labelCol)), f, w)
.map { case Row(label: Double, feature: Double, weights: Double) =>
(label, feature, weights)
.map { case Row(label: Double, feature: Double, weight: Double) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is better to use weight rather than weights in there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching this!

@vectorijk vectorijk changed the title [SPARK-10592] [WIP] [ML] [PySpark] Deprecate weights and use coefficients instead in ML models [SPARK-10592] [ML] [PySpark] Deprecate weights and use coefficients instead in ML models Oct 30, 2015
@vectorijk
Copy link
Contributor Author

didn't deprecate weights under spark/ml related to weights in Layer.scala. cause I think weights do mean weight in spark/ml/ann/Layer.scala

@SparkQA
Copy link

SparkQA commented Oct 30, 2015

Test build #44675 has finished for PR 9311 at commit 5cfa573.

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

@SparkQA
Copy link

SparkQA commented Oct 30, 2015

Test build #44680 has finished for PR 9311 at commit f1050ae.

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

mengxr
mengxr reviewed Nov 2, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/r/SparkRWrappers.scala
@@ -51,13 +51,22 @@ private[r] object SparkRWrappers {
pipeline.fit(df)
}

@deprecated("Use getModelCoefficients instead.", "1.6.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to put the deprecation message because this is a private object. Just renaming the method should be sufficient.

Copy link
Member

Choose a reason for hiding this comment

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

Since it's private, why don't we remove it entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dbtsai I was a little behind on this.
Solved in #9426

@mengxr
Copy link
Contributor

mengxr commented Nov 2, 2015

LGTM except one minor issue. @dbtsai Do you have time to make another pass?

dbtsai
dbtsai reviewed Nov 2, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/ml/classification/LogisticRegression.scala
val intercept: Double)
extends ProbabilisticClassificationModel[Vector, LogisticRegressionModel]
with LogisticRegressionParams {

@deprecated("Use coefficients instead.", "1.6.0")
def weights: Vector = coefficients
Copy link
Member

Choose a reason for hiding this comment

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

Will this cause binary compatibility issue?

@dbtsai
Copy link
Member

dbtsai commented Nov 2, 2015

This looks good to me, but I wonder if changing from val weights into def weights: Vector = coefficients will cause any binary compatibility issue.

@mengxr
Copy link
Contributor

mengxr commented Nov 3, 2015

No, that is a compatible change.

@dbtsai
Copy link
Member

dbtsai commented Nov 3, 2015

Thanks. Merged into master.

@asfgit asfgit closed this in c020f7d Nov 3, 2015
@mengxr
Copy link
Contributor

mengxr commented Nov 3, 2015

@dbtsai The comment on LogisticRegressionAggregator is not addressed yet. Could you send a PR to fix it?

@vectorijk
Copy link
Contributor Author

Thanks @mengxr @dbtsai !

@yanboliang yanboliang mentioned this pull request Nov 3, 2015
Closed
@vectorijk
Copy link
Contributor Author

vectorijk commented May 25, 2016 •
edited
Loading

@bharath-official Try to use coefficients instead. This is warning information. Did you get error message? I think it will be fine cause we didn't remove weights.

@bharath-official
Copy link

bharath-official commented May 25, 2016 •
edited
Loading

@vectorijk Yes you are right Kai it is a warning that is thrown and not an error. The function also seems to work as expected. Since this use case gets showcased in one of our user documentation, I wanted to keep it clean by eliminating the warnings too. When you say use coefficients instead, Sorry I didnt get the context. My understanding was that, the transform function that I am calling on model in turn uses weights and thus this cant be fixed from the callers side. This needs to be fixed in the source code of transform function. Correct me if I am wrong.

@vectorijk
Copy link
Contributor Author

@bharath-official You're right.
Aha. Like you said, it should work fine if this warning message just shows during the process of calling model.

@bharath-official
Copy link

@vectorijk Yes you are right. It works fine but throws a warning which I am trying to avoid. Would it make sense if I go ahead and look at the transform code in Spark and change weights to coefficients and send you a PR?

@vectorijk
Copy link
Contributor Author

@bharath-official I don't think it's really necessary because it was changed in Spark 2.0

@bharath-official
Copy link

@vectorijk Oh great then! That makes sense. Thanks for your inputs Kai.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vectorijk @SparkQA @mengxr @dbtsai @bharath-official @yanboliang