Skip to content

Conversation

@ehsanmok
Copy link
Contributor

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fix the indentations for the parameters. There is no problem about the @Since tag.

@ehsanmok
Copy link
Contributor Author

I suspected that might be an issue, but the indentations were automatic in intelliJ!

@mengxr
Copy link
Contributor

mengxr commented Oct 20, 2015

@ehsanmok Could you fix it manually and resolve conflicts with master? The default IntelliJ code style doesn't follow the Spark code style guide. It is important to keep the code style consistent across the codebase.

@mengxr
Copy link
Contributor

mengxr commented Oct 20, 2015

ok to test

@SparkQA
Copy link

SparkQA commented Oct 21, 2015

Test build #44020 has finished for PR 8729 at commit 40dc4f2.

  • This patch fails Scala style tests.
  • This patch does not merge cleanly.
  • This patch adds the following public classes (experimental):
    • final class DecisionTreeRegressor @Since("1.4.0") (@Since("1.4.0") override val uid: String)
    • final class GBTRegressor @Since("1.4.0") (@Since("1.4.0") override val uid: String)
    • final class GBTRegressionModel @Since("1.4.0") (
    • class IsotonicRegression @Since("1.5.0") (@Since("1.5.0") override val uid: String)
    • class LinearRegression @Since("1.3.0") (@Since("1.3.0") override val uid: String)
    • final class RandomForestRegressor @Since("1.4.0") (@Since("1.4.0") override val uid: String)
    • class CrossValidator @Since("1.2.0") (@Since("1.2.0") override val uid: String)
    • class TrainValidationSplit @Since("1.5.0") (@Since("1.5.0") override val uid: String)

@ehsanmok
Copy link
Contributor Author

@mengxr Sure! should be ok now.

@SparkQA
Copy link

SparkQA commented Oct 21, 2015

Test build #44023 has finished for PR 8729 at commit eb4f580.

  • This patch fails Scala style tests.
  • This patch does not merge cleanly.
  • This patch adds the following public classes (experimental):
    • final class DecisionTreeRegressor @Since("1.4.0") (@Since("1.4.0") override val uid: String)
    • final class GBTRegressor @Since("1.4.0") (@Since("1.4.0") override val uid: String)
    • final class GBTRegressionModel @Since("1.4.0") (
    • class IsotonicRegression @Since("1.5.0") (@Since("1.5.0") override val uid: String)
    • class LinearRegression @Since("1.3.0") (@Since("1.3.0") override val uid: String)
    • final class RandomForestRegressor @Since("1.4.0") (@Since("1.4.0") override val uid: String)
    • class CrossValidator @Since("1.2.0") (@Since("1.2.0") override val uid: String)
    • class TrainValidationSplit @Since("1.5.0") (@Since("1.5.0") override val uid: String)

@ehsanmok
Copy link
Contributor Author

@mengxr @yu-iskw Which line in DecisionTreeRegressor.scala it's referring to? the log is not very helpful. I've looked at the coding style as well as the code base, but cannot find what that is!

@mengxr
Copy link
Contributor

mengxr commented Oct 21, 2015

Ignore the comment from @SparkQA . You need to first merge current master and resolve conflicts, and then run dev/lint-scala to check code style on your local computer.

@SparkQA
Copy link

SparkQA commented Oct 21, 2015

Test build #44085 has finished for PR 8729 at commit 338bbf8.

  • This patch fails Scala style tests.
  • This patch does not merge cleanly.
  • This patch adds the following public classes (experimental):
    • final class DecisionTreeRegressor @Since("1.4.0") (@Since("1.4.0") override val uid: String)
    • final class GBTRegressor @Since("1.4.0") (@Since("1.4.0") override val uid: String)
    • final class GBTRegressionModel @Since("1.4.0") (
    • class IsotonicRegression @Since("1.5.0") (@Since("1.5.0") override val uid: String)
    • class LinearRegression @Since("1.3.0") (@Since("1.3.0") override val uid: String)
    • final class RandomForestRegressor @Since("1.4.0") (@Since("1.4.0") override val uid: String)
    • class CrossValidator @Since("1.2.0") (@Since("1.2.0") override val uid: String)
    • class TrainValidationSplit @Since("1.5.0") (@Since("1.5.0") override val uid: String)

@ehsanmok
Copy link
Contributor Author

@mengxr sorry for taking your time, but what's the issue now? everything is updated and all dev/lint-scala tests have passed locally!

@SparkQA
Copy link

SparkQA commented Oct 21, 2015

Test build #44097 has finished for PR 8729 at commit 45472f0.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * final class DecisionTreeRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * final class GBTRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * final class GBTRegressionModel @Since(\"1.4.0\") (\n * class IsotonicRegression @Since(\"1.5.0\") (@Since(\"1.5.0\") override val uid: String)\n * class LinearRegression @Since(\"1.3.0\") (@Since(\"1.3.0\") override val uid: String)\n * final class RandomForestRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * class CrossValidator @Since(\"1.2.0\") (@Since(\"1.2.0\") override val uid: String)\n * class TrainValidationSplit @Since(\"1.5.0\") (@Since(\"1.5.0\") override val uid: String)\n

@SparkQA
Copy link

SparkQA commented Oct 21, 2015

Test build #44099 has finished for PR 8729 at commit 353f23a.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * final class DecisionTreeRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * final class GBTRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * class IsotonicRegression @Since(\"1.5.0\") (@Since(\"1.5.0\") override val uid: String)\n * class LinearRegression @Since(\"1.3.0\") (@Since(\"1.3.0\") override val uid: String)\n * final class RandomForestRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * class CrossValidator @Since(\"1.2.0\") (@Since(\"1.2.0\") override val uid: String)\n * class TrainValidationSplit @Since(\"1.5.0\") (@Since(\"1.5.0\") override val uid: String)\n * case class First(child: Expression, ignoreNullsExpr: Expression) extends DeclarativeAggregate\n * case class Last(child: Expression, ignoreNullsExpr: Expression) extends DeclarativeAggregate\n * case class First(\n * case class FirstFunction(\n * case class Last(\n * case class LastFunction(\n

@ehsanmok
Copy link
Contributor Author

After dev/lint-scala should I include the build/sbt-launch-0.13.7.jar.part as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolve this conflict.

@yu-iskw
Copy link
Contributor

yu-iskw commented Oct 23, 2015

You don't need to include it. You should fix style errors which are pointed out by dev/lint-scala. According to the Jenkins result, there are some White space at end of line errors.

This is a result of grep in your branch, checking out it to my local computer.

> grep -n -iRe "\s\s*$" mllib/ | grep "\.scala"
mllib//src/main/scala/org/apache/spark/ml/regression/DecisionTreeRegressor.scala:59:  @Since("1.4.0")
mllib//src/main/scala/org/apache/spark/ml/regression/GBTRegressor.scala:184:
mllib//src/main/scala/org/apache/spark/ml/regression/GBTRegressor.scala:201:
mllib//src/main/scala/org/apache/spark/ml/regression/GBTRegressor.scala:206:

@SparkQA
Copy link

SparkQA commented Oct 23, 2015

Test build #44256 has finished for PR 8729 at commit 4cbb69f.

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

@ehsanmok
Copy link
Contributor Author

@yu-iskw Thank you very much! I don't know why I missed those in git diff. Btw that was a good learning experience indeed.

@yu-iskw
Copy link
Contributor

yu-iskw commented Oct 23, 2015

@ehsanmok thank you for the update. Did you update the branch with git merge? If so, I think you should use git rebase to commit to the updated upstream head. Because you should separate your patchs and master updates. The last your commit seems to involve the master update. Could you revert the last commit and then git rebase master?

@ehsanmok
Copy link
Contributor Author

@yu-iskw I did 'git merge' but couldn't rebase for some reason! It took me very long time to understand where the issue was! didn't want to make all these commits. Don't know why this simple thing has become a headache to me!

@SparkQA
Copy link

SparkQA commented Oct 23, 2015

Test build #44271 has finished for PR 8729 at commit 0fc6619.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * final class DecisionTreeRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * final class GBTRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * class IsotonicRegression @Since(\"1.5.0\") (@Since(\"1.5.0\") override val uid: String)\n * class LinearRegression @Since(\"1.3.0\") (@Since(\"1.3.0\") override val uid: String)\n * final class RandomForestRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * class CrossValidator @Since(\"1.2.0\") (@Since(\"1.2.0\") override val uid: String)\n * class TrainValidationSplit @Since(\"1.5.0\") (@Since(\"1.5.0\") override val uid: String)\n

@SparkQA
Copy link

SparkQA commented Oct 24, 2015

Test build #44273 has finished for PR 8729 at commit a9ed325.

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

@yu-iskw
Copy link
Contributor

yu-iskw commented Oct 24, 2015

@ehsanmok thank you for the update. I'm afraid your repo still includes the master updates. As you can see at https://github.com/apache/spark/pull/8729/files, there are many files that are not related this issue.

I think making a new branch is a easy way to solve this problem. First, make a new branch whose name is same as JIRA-10266 based on the master. And then, cherry pick your patchs related to only this issue to the new branch. Instead of, It would be also great to patch what you changed manually. After that, push the new repo to JIRA-10266 on GitHub, that is, replace it. Otherwise, I could also send a PR to you.

@ehsanmok
Copy link
Contributor Author

@yu-iskw Thanks for your kind suggestions. Sure, I am going to do that asap.

@ehsanmok
Copy link
Contributor Author

@yu-iskw The problem I kept having is my master was polluted with unwanted commits and so I reset hard and pulled from upstream/master and pushed master updates as "update master", then I checked out on JIRA-10266 branch and reset that as well and pulled updates from master and resolved conflicts and pushed as the last commit. What do you think about this if I don't want to have master updates here?

@SparkQA
Copy link

SparkQA commented Oct 27, 2015

Test build #44449 has finished for PR 8729 at commit 27c9cc8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * final class DecisionTreeRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * final class GBTRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * class IsotonicRegression @Since(\"1.5.0\") (@Since(\"1.5.0\") override val uid: String)\n * class LinearRegression @Since(\"1.3.0\") (@Since(\"1.3.0\") override val uid: String)\n * final class RandomForestRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * class CrossValidator @Since(\"1.2.0\") (@Since(\"1.2.0\") override val uid: String)\n * class TrainValidationSplit @Since(\"1.5.0\") (@Since(\"1.5.0\") override val uid: String)\n

@yu-iskw
Copy link
Contributor

yu-iskw commented Oct 28, 2015

@ehsanmok it would be great not to have the master update commits in your branch. Personally, I think you should use git rebase instead of git pull.

Could you please close this PR? I'll send a new one about this issue. Since I couldn't send a PR to improve your branch. I'd like to separate resolving this issue and explaining git operations. It would be great to do that on #8728.

@ehsanmok
Copy link
Contributor Author

@yu-iskw Sure, I'll close it. But do you mean, first test #8728 then do this one?

@yu-iskw
Copy link
Contributor

yu-iskw commented Oct 28, 2015

@ehnalis could you wait for the first test on #8728. We should know if there are any conflicts with the master or not. If there are any ones, we need to update your #8728 PR with git rebase. Anyway, let me expnain how to update your branch again on #8728.

Just to be sure, I'll work on SPARK-10266. So you shouldn't resend a new PR about SPARK-10266.
Thank you for your contribution!

@ehsanmok ehsanmok closed this Oct 28, 2015
@ehsanmok ehsanmok deleted the JIRA-10266 branch November 8, 2015 17:43
asfgit pushed a commit that referenced this pull request Dec 2, 2015
cc mengxr noel-smith

I worked on this issues based on #8729.
ehsanmok  thank you for your contricution!

Author: Yu ISHIKAWA <[email protected]>
Author: Ehsan M.Kermani <[email protected]>

Closes #9338 from yu-iskw/JIRA-10266.

(cherry picked from commit de07d06)
Signed-off-by: Xiangrui Meng <[email protected]>
asfgit pushed a commit that referenced this pull request Dec 2, 2015
cc mengxr noel-smith

I worked on this issues based on #8729.
ehsanmok  thank you for your contricution!

Author: Yu ISHIKAWA <[email protected]>
Author: Ehsan M.Kermani <[email protected]>

Closes #9338 from yu-iskw/JIRA-10266.
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