Skip to content

Conversation

@quintinali
Copy link
Contributor

No description provided.

@asfgit
Copy link

asfgit commented Jun 29, 2018

Can one of the admins verify this patch?

Copy link
Member

@lewismc lewismc left a comment

Choose a reason for hiding this comment

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

There are several comments.
Please make sure that the files are removed from core if they are being built in to ranking.
This is looking good.

@@ -0,0 +1,215 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Please ensure that license header is formatted correctly.

Also ensure that the code formatting matches what is present in the other pom.xml files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some files will be moved to ranking module in the next step, such as RankingTrainData.java because I worked on the expert provided train data now.

</resource>
</resources>

<plugins>
Copy link
Member

Choose a reason for hiding this comment

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

We do not need the appassembler-maven-plugin configuration

</plugin>

<plugin>
<groupId>org.apache.maven.plugins</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

We do not need the maven-shade-plugin configuration

@@ -0,0 +1,50 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

We do not need this

@@ -0,0 +1,57 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Make sure that code formatting is 2 space indents


public static void main(String[] args) {
SparkFormatter sf = new SparkFormatter();
sf.toSparkSVMformat("C:/mudrodCoreTestData/rankingResults/inputDataForSVM.csv", "C:/mudrodCoreTestData/rankingResults/inputDataForSVM_spark.txt");
Copy link
Member

Choose a reason for hiding this comment

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

This cannot be hardcoded.

* into a user specified directory.
*/
public void process() {
public void convert2TrainSet() {
Copy link
Member

Choose a reason for hiding this comment

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

Change convert2TrainSet to convertToTrainSet

for (int i = 1; i < arr.length - row; i++) {
List<String> colList = new ArrayList<String>(); // create vector to store all values inside of a column, which is stored inside 2D vector
for (int col = 0; col < arr[0].length - 1; col++) // Columns go until the next to last column
for (int col = 1; col < arr[0].length - 2; col++) // Columns go until the next to last column
Copy link
Member

Choose a reason for hiding this comment

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

Why has this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before Yongyao deleted the first column of the experts provided data manually and then invoked the function to process data.

Copy link
Member

Choose a reason for hiding this comment

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

wow, ok thank you for fixing.


public RankTrainDataFactory(Properties props, ESDriver es, SparkDriver spark) {
super(props, es, spark);
// TODO Auto-generated constructor stub
Copy link
Member

Choose a reason for hiding this comment

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

Remove TODO

}

// start session
// mode: overwrite or append
Copy link
Member

Choose a reason for hiding this comment

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

Remove comments

@lewismc
Copy link
Member

lewismc commented Jun 29, 2018

Also @quintinali please make sure that you test this code with monthly input logs.

@lewismc
Copy link
Member

lewismc commented Jul 3, 2018

@quintinali can you update the PR with the changes ? Thanks

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.

3 participants