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.

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

Fix jvm tests. #4903

Open
wants to merge 3 commits into
base: master
from

Conversation

@trams
Copy link
Contributor

commented Sep 27, 2019

2 things are done

  1. all data files are read from resources not by relative path in a source tree (this enables easy move to SBT or other build system)
  2. fixed DTDs

This is an extraction from this pull request #4687

trams added 3 commits Jul 18, 2019
To achieve this I had to move the train/test data to the main package
and replace all hardcoded occurences of "../../demo" in the tests.

Original author is Sergei Lebedev
criteo-forks@d10b607
"-//Puppy Crawl//DTD Check Configuration 1.3//EN"
"http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
"-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
"https://checkstyle.org/dtds/configuration_1_3.dtd">

This comment has been minimized.

Copy link
@CodingCat

CodingCat Sep 30, 2019

Member

just question, what happened to them?

This comment has been minimized.

Copy link
@trams

trams Sep 30, 2019

Author Contributor

Frankly I do not know.
It may be worth reporting to the owner of puppycrawl.com

maybe_makedirs("xgboost4j/src/test/resources")
for file in glob.glob("../demo/regression/machine.txt.t*"):

This comment has been minimized.

Copy link
@CodingCat

CodingCat Sep 30, 2019

Member

if machine.txt.t* is only used for xgboost4j-spark, why we put it in xgboost4j?

@@ -20,10 +20,9 @@ import java.io.File
import java.util.Arrays

import scala.io.Source
import ml.dmlc.xgboost4j.scala.{Classification, DMatrix, Regression}

This comment has been minimized.

Copy link
@CodingCat

CodingCat Sep 30, 2019

Member

why we have many adding of imports but never use them?

This comment has been minimized.

Copy link
@trams

trams Sep 30, 2019

Author Contributor

I checked. We use all of those added imports.
One needs to understand why they are not in this diff

This comment has been minimized.

Copy link
@CodingCat

CodingCat Sep 30, 2019

Member

ah, I think you moved Classification and Regression to xgboost4j leading to these changes

@@ -14,12 +14,14 @@
limitations under the License.
*/

package ml.dmlc.xgboost4j.scala.spark
package ml.dmlc.xgboost4j.scala

This comment has been minimized.

Copy link
@CodingCat

CodingCat Sep 30, 2019

Member

if you move this file to xgboost4j, there is a significant part duplicate with TrainTestData.java and Classification.java

additionally, most of methods here are only called by spark package, why we need to move?

This comment has been minimized.

Copy link
@trams

trams Sep 30, 2019

Author Contributor

This is a good point. I remember that I moved it because there was at least one test in xgboost4j which used this data but now I can't seem to figure out which one.

So I will probably try to move it back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.