Skip to content

Conversation

@ehsanmok
Copy link
Contributor

Here is my first commit.

@yu-iskw
Copy link
Contributor

yu-iskw commented Oct 28, 2015

@ehsanmok according to the warning message, we have any conflicts with the master. You need to update your PR. So I'd like to explain how to resolve that. And I'd like to review this PR after the first test pass.

When I have conflicts with the master after sending a PR, I always do the following operations.

# now, you are on your branch. In this case, it's `SinceAnn`.
# where, `upstream` is the `apache/spark` repository.
# if you set the different name on your local computer, replace `upstream` with the name..
git fetch upstream
git rebase upstream/master

# resolve the conflicts

# make sure to check coding style warnings before pushing it to github.
./dev/lint-scala

# push it to github
# where, `origin` is your github repository. In this case, It's `ehsanmok/spark`.
git push -f origin SinceAnn

If you have any questions, please don't hesitate to ask.

@ehsanmok
Copy link
Contributor Author

@yu-iskw Thank you very much for the detailed explanations! I just want to make sure that when I resolve the conflicts (also "Scalastyle checks passed") and do git rebase --continue, it shows unmerged files, so should I git add <files> and then git merge SinceAnn --no-ff or with --no-commit, before forced push?

@yu-iskw
Copy link
Contributor

yu-iskw commented Oct 28, 2015

@ehsanmok it depends on the situation. Since I don't know what kind of files appeared as unmerged files, it's hard to decide if you need to add them or not.

@ehsanmok
Copy link
Contributor Author

@yu-iskw should be fine now. Please test it.

@yu-iskw
Copy link
Contributor

yu-iskw commented Oct 28, 2015

ok to test

@yu-iskw
Copy link
Contributor

yu-iskw commented Oct 28, 2015

@mengxr can you run the test when you have time.
BTW, does everyone have the privilege to run tests in this case?

@mengxr
Copy link
Contributor

mengxr commented Nov 2, 2015

ok to test

@mengxr
Copy link
Contributor

mengxr commented Nov 2, 2015

add to whitelist

@SparkQA
Copy link

SparkQA commented Nov 2, 2015

Test build #44842 has finished for PR 8728 at commit 4f7c36f.

  • 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

@SparkQA
Copy link

SparkQA commented Nov 4, 2015

Test build #44989 has finished for PR 8728 at commit 175d75e.

  • 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

Copy link
Contributor

Choose a reason for hiding this comment

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

insert a blank line above the @SInCE tag. Please do likewise at other tags.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be since 1.4.0.

@yu-iskw
Copy link
Contributor

yu-iskw commented Nov 4, 2015

@ehsanmok thanks for the update. We should add @Since to public classes and public methods

  • DecisionTreeRegressionModel
  • GBTRegressionModel
  • IsotonicRegressionModel
  • LinearRegression.setWeightCol and LinearRegression.setSolver
  • LinearRegressionModel
  • LinearRegressionTrainingSummary
  • RandomForestRegressionModel

@SparkQA
Copy link

SparkQA commented Nov 4, 2015

Test build #45042 has finished for PR 8728 at commit beaa3aa.

  • 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

@ehsanmok
Copy link
Contributor Author

ehsanmok commented Nov 4, 2015

@yu-iskw Thanks for the correction. I think, DecisionTreeRegressionModel is a private[ml] class, right? so is GBTRegressionModel, IsotonicRegressionModel, LinearRegressionModel and RandomForestRegressionModel. And LinearRegressionTrainingSummary is a public method in a private class, so needs a tag?

I assume LinearRegression.setWeightCol and LinearRegression.setSolver should be tagged as 1.6.0, right?

@yu-iskw
Copy link
Contributor

yu-iskw commented Nov 4, 2015

@ehsanmok

  • DecisionTreeRegressionModel is a public class. The constructor paramter is private[ml].
  • GBTRegressionModel and so on are also public classes.
  • setWeightCol and setSolver should be since 1.6.0.

@yu-iskw
Copy link
Contributor

yu-iskw commented Nov 4, 2015

@ehsanmok thanks for the update!

@ehsanmok
Copy link
Contributor Author

ehsanmok commented Nov 4, 2015

@yu-iskw Thank you very much for your patients and mentoring me. I wouldn't have imagined how much I would learn by doing this starter task.

@SparkQA
Copy link

SparkQA commented Nov 4, 2015

Test build #45047 has finished for PR 8728 at commit fa61502.

  • 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

@yu-iskw
Copy link
Contributor

yu-iskw commented Nov 4, 2015

@ehsanmok no problem. I appreciate your first contribution!

@mengxr LGTM

@yu-iskw
Copy link
Contributor

yu-iskw commented Nov 4, 2015

Jenkins, retest this please

@yu-iskw
Copy link
Contributor

yu-iskw commented Nov 4, 2015

Jenkins, test this please.

@yu-iskw
Copy link
Contributor

yu-iskw commented Nov 4, 2015

@mengxr there is something wrong with Jenkins. It failed to fetch source code from github 3 times in a row. Could you help us?

cc @JoshRosen

@mengxr
Copy link
Contributor

mengxr commented Nov 5, 2015

test this please

@SparkQA
Copy link

SparkQA commented Nov 5, 2015

Test build #45128 has finished for PR 8728 at commit 55e1317.

  • 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

@mengxr
Copy link
Contributor

mengxr commented Nov 5, 2015

Merged into master. Thanks!

@asfgit asfgit closed this in f80f7b6 Nov 5, 2015
@ehsanmok ehsanmok deleted the SinceAnn branch November 8, 2015 17:42
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