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

[jvm-packages]Adds the ability to pass multiple custom evaluation #3544

Closed
wants to merge 14 commits into from
Closed

[jvm-packages]Adds the ability to pass multiple custom evaluation #3544

wants to merge 14 commits into from

Conversation

Helw150
Copy link

@Helw150 Helw150 commented Aug 1, 2018

Putting this here to gain access to the CI. This PR is based off updating and old fork of XGBoost which added this functionality to the current master state, so may have errors from merge conflict.

@Helw150
Copy link
Author

Helw150 commented Aug 1, 2018

Additionally, I ran ScalaFmt and google-java-fmt - which passed the XGBoost official style checks, but I am not sure if those meet the standards of XGBoost open source in practice.

@hcho3 hcho3 mentioned this pull request Aug 1, 2018
Closed
@hcho3
Copy link
Collaborator

hcho3 commented Aug 1, 2018

This is great. Does your fork also have watchlist feature for XGBoost4J-Spark?

@hcho3 hcho3 mentioned this pull request Aug 1, 2018
Closed
@Helw150 Helw150 force-pushed the adds-multiple-eval branch from 3fd3812 to 52c60b4 Compare August 1, 2018 21:05
@CodingCat
Copy link
Member

would you please avoid changing format in the same PR, though there are some style problems especially for Java code,

we may focus on your watch list thing in this PR

@CodingCat CodingCat changed the title [WIP] Adds the ability to pass multiple custom evaluation [jvm-packages][WIP] Adds the ability to pass multiple custom evaluation Aug 1, 2018
@Helw150 Helw150 force-pushed the adds-multiple-eval branch from 52c60b4 to 51071c2 Compare August 1, 2018 21:44
@Helw150
Copy link
Author

Helw150 commented Aug 1, 2018

@CodingCat I just styled with google-java-format in order to pass the linter as quickly as possible rather than doing manual nits. I can try to revert as many style changes as possible if you'd like.

@Helw150
Copy link
Author

Helw150 commented Aug 1, 2018

Alternatively, let me know if you guys have a preferred formatter or a more specific ruleset of your linter I should follow. I try to leave as much as possible to linters and formatters style wise.

@CodingCat
Copy link
Member

I don't think the current linter is checking jvm/* java has no style checker now...scala is using scala-style-check plugin....

@Helw150
Copy link
Author

Helw150 commented Aug 1, 2018

Do you guys have a style guide then? I'm working on this as a side project, so haven't particularly set up a good dev environment for iterating on this so anything that simplifies my process to follow y'alls ruleset is much appreciated!

@Helw150
Copy link
Author

Helw150 commented Aug 1, 2018

The java did seem to be linting with Checkstyle(that was my motivation for using a formatter), so I just adhered to the linting rules Checkstyle imposed.

@CodingCat
Copy link
Member

what's the error you received? if you don't change any format, I believe it will not break with stylistic errors (as all the other PRs are successfully passing tests)

@CodingCat
Copy link
Member

our scala style is essentially a subset (removed some spark related thing) of https://github.com/databricks/scala-style-guide

@Helw150
Copy link
Author

Helw150 commented Aug 1, 2018 •
edited
Loading

@CodingCat

[ERROR] src/main/java/ml/dmlc/xgboost4j/java/XGBoost.java:[108] (sizes) LineLength: Line is longer than 100 characters (found 114).
[ERROR] src/main/java/ml/dmlc/xgboost4j/java/XGBoost.java:[139] (indentation) Indentation: 'method def' child have incorrect indentation level 6, expected level should be 4.
[ERROR] src/main/java/ml/dmlc/xgboost4j/java/XGBoost.java:[139] (sizes) LineLength: Line is longer than 100 characters (found 119).
[ERROR] src/main/java/ml/dmlc/xgboost4j/java/XGBoost.java:[207] (indentation) Indentation: 'for' child have incorrect indentation level 18, expected level should be 12.
[ERROR] src/main/java/ml/dmlc/xgboost4j/java/XGBoost.java:[207] (indentation) Indentation: 'member def type' have incorrect indentation level 18, expected level should be 12.

I'll just fix these manually. Running the Google Style guide formatter fixes all these Lint issues, but your style guide in practice seems to differ from the linter in that my above changes met lint. Sorry for the inconvenience!

@CodingCat
Copy link
Member

CodingCat commented Aug 1, 2018 •
edited
Loading

still confused why all the other PRs didn't touch java can pass lint checker (are you referring to Travis CI's lint checker), but you have such an issue

can you just put stylistic issue aside and implement functionalities? or just follow what it says about style manually (as you said)?

@CodingCat
Copy link
Member

maybe lint works as "check when there is any modification in the directory" since you add a new file there

@Helw150 Helw150 force-pushed the adds-multiple-eval branch 5 times, most recently from cac435e to 517dcdd Compare August 7, 2018 15:53
@Helw150
Copy link
Author

Helw150 commented Aug 7, 2018

@CodingCat @hcho3 I don't believe the PR-merge CI errors stem from my changes as it stems from a C error and seems to be affecting a few other open PR's. Am I correct in that?

@hcho3
Copy link
Collaborator

hcho3 commented Aug 7, 2018

@Helw150 Correct, I'm currently looking into it.

@Helw150
Copy link
Author

Helw150 commented Aug 7, 2018

@CodingCat You seem to be the one to look to for review on this one? Whenever you get a chance to review, I'd be super grateful :)

@Helw150 Helw150 force-pushed the adds-multiple-eval branch from da154e0 to bc2620b Compare August 8, 2018 15:12
@Helw150
Copy link
Author

Helw150 commented Aug 10, 2018

Ping @CodingCat or @hcho3 for review :)

@CodingCat
Copy link
Member

@Helw150 we are focusing on 0.8 release, will look at it later

@Helw150
Copy link
Author

Helw150 commented Aug 10, 2018 •
edited
Loading

Great, just making sure it made it on your radar. I'll add change WIP->Ready For Review as a reminder. Thanks for the help

@Helw150 Helw150 changed the title [jvm-packages][WIP] Adds the ability to pass multiple custom evaluation [jvm-packages][Ready for Review] Adds the ability to pass multiple custom evaluation Aug 13, 2018
@Helw150
Copy link
Author

Helw150 commented Aug 17, 2018

@CodingCat @hcho3 Just a note, Next week is the last week of the internship where I have been working on adding this functionality as a side project. I would really appreciate it if one of you has a bit of time to review.

Sorry to ping again 😓

@CodingCat
Copy link
Member

Sorry, No guarantee on that, but we will try....one thing to note is that none of us work on this as full time, instead, we spent time which we squeeze from our daily jobs

@Helw150
Copy link
Author

Helw150 commented Aug 17, 2018

Yup, totally understand as it's the same for me on this PR. Just wanted to make sure it was still on your radar as I had seen new PR's get reviewed and merged as I know it's easy to forget things when they aren't on your roadmap!

If it can't get reviewed, no worries. It's living happily on our fork internally, so for our purposes no harm no foul if we can't get this merged.

@hcho3
Copy link
Collaborator

hcho3 commented Aug 17, 2018 •
edited
Loading

@Helw150 We do aim to get all pull requests merged eventually, so that the list doesn't exceed one page.

@CodingCat Wish I knew a bit about Scala so that I can help you here. The Java land is still quite new to me hehe

@Helw150
Copy link
Author

Helw150 commented Aug 17, 2018

@hcho3 No worries. I'm in no huge rush as long as it's on your list 👍

CodingCat
CodingCat reviewed Aug 24, 2018
View reviewed changes
Copy link
Member

@CodingCat CodingCat left a comment

Choose a reason for hiding this comment

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

generally looks good, left some comments and we need to figure out that metricsOut thing

jvm-packages/xgboost4j/src/main/java/ml/dmlc/xgboost4j/java/XGBoost.java Show resolved Hide resolved
jvm-packages/xgboost4j/src/main/scala/ml/dmlc/xgboost4j/scala/XGBoost.scala Outdated
XGBoostError,
BoosterResults,
IEvaluation
}
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary multi-lining

jvm-packages/xgboost4j/src/main/scala/ml/dmlc/xgboost4j/scala/XGBoost.scala Outdated
evals: Array[IEvaluation] = null,
earlyStoppingRound: Int = 0,
booster: Booster = null
): BoosterResults = {
Copy link
Member

Choose a reason for hiding this comment

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

consolidate line 45 and 46

jvm-packages/xgboost4j/src/main/scala/ml/dmlc/xgboost4j/scala/XGBoost.scala Outdated Show resolved Hide resolved
jvm-packages/xgboost4j/src/main/scala/ml/dmlc/xgboost4j/scala/XGBoost.scala Outdated
obj,
evals,
earlyStoppingRound,
jBooster
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need make one parameter per line

jvm-packages/xgboost4j/src/main/scala/ml/dmlc/xgboost4j/scala/XGBoost.scala Outdated
obj,
evals,
earlyStoppingRound,
booster
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary multi-lining

jvm-packages/xgboost4j/src/main/scala/ml/dmlc/xgboost4j/scala/XGBoost.scala Outdated
@@ -52,22 +95,80 @@ object XGBoost {
watches: Map[String, DMatrix] = Map(),
metrics: Array[Array[Float]] = null,
obj: ObjectiveTrait = null,
eval: EvalTrait = null,
eval: IEvaluation = null,
Copy link
Member

Choose a reason for hiding this comment

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

EvalTrait is not used at all?

jvm-packages/xgboost4j/src/main/scala/ml/dmlc/xgboost4j/scala/XGBoost.scala Outdated
@@ -94,11 +195,20 @@ object XGBoost {
nfold: Int = 5,
metrics: Array[String] = null,
obj: ObjectiveTrait = null,
eval: EvalTrait = null): Array[String] = {
eval: EvalTrait = null
): Array[String] = {
Copy link
Member

Choose a reason for hiding this comment

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

consolidate 198 and 199

jvm-packages/xgboost4j/src/main/scala/ml/dmlc/xgboost4j/scala/XGBoost.scala Outdated
obj,
evals,
earlyStoppingRound,
booster
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary multi-lining

jvm-packages/xgboost4j/src/main/scala/ml/dmlc/xgboost4j/scala/XGBoost.scala Outdated
evals: Array[IEvaluation] = null,
earlyStoppingRound: Int = 0,
booster: Booster = null
): Booster = {
Copy link
Member

Choose a reason for hiding this comment

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

consolidate 157 and 158

@CodingCat
Copy link
Member

any update on this?

@Helw150
Copy link
Author

Helw150 commented Sep 2, 2018 •
edited
Loading

@CodingCat As I said a bit ago, the internship I was working on this as a side project for ended so I don't have nearly as much time to make fixes for the next couple of months. I'll try to make changes based on your comments in the next few weeks, but I don't have any explicit time on my calendar to work on this while at school.

@CodingCat CodingCat changed the title [jvm-packages][Ready for Review] Adds the ability to pass multiple custom evaluation [jvm-packages]Adds the ability to pass multiple custom evaluation Sep 13, 2018
@hcho3
Copy link
Collaborator

hcho3 commented Sep 23, 2018

Please ignore the failed test continuous-integration/jenkins/pr-head. (This was due to misconfiguration of Jenkins CI server.) Only continuous-integration/jenkins/pr-merge is relevant.

@Helw150
Copy link
Author

Helw150 commented Oct 2, 2018

@CodingCat This should be in a state for you to give another look. I fixed all the issues you had, the primary thing I am uncertain on is how I should handle the one of the lines you said shouldn't have one argument per line.

The style guide you linked earlier has the following as it's standard for long method invocations, so for now I've reverted line 54-64 to a single parameter per line to meet the style guide.

  • For method and class constructor invocations, use 2 space indentation for its parameters and put each in each line when the parameters don't fit in two lines.

    foo(
  someVeryLongFieldName,  // 2 space indent here
  andAnotherVeryLongFieldName,
  "this is a string",
  3.1415)

new Bar(
  someVeryLongFieldName,  // 2 space indent here
  andAnotherVeryLongFieldName,
  "this is a string",
  3.

@Helw150 Helw150 force-pushed the adds-multiple-eval branch from 82799b8 to 5daced5 Compare October 2, 2018 17:03
@hcho3
Copy link
Collaborator

hcho3 commented Mar 22, 2022

Closing as stale

@hcho3 hcho3 closed this Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodingCat CodingCat CodingCat left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Helw150 @hcho3 @CodingCat