Skip to content

Conversation

@zhengruifeng
Copy link
Contributor

What changes were proposed in this pull request?

1, remove unused extractInstances and extractLabeledPoints in Predictor;
2, remove unused checkNonNegativeWeight in function;
3, move getNumClasses from Clasifier to DatasetUtils;
4, move getNumFeatures from MetadataUtils to DatasetUtils;

Why are the changes needed?

to unify to methods

Does this PR introduce any user-facing change?

No

How was this patch tested?

existing testsuites

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

This is just refactoring - shouldn't change behavior? seems OK

Copy link
Member

Choose a reason for hiding this comment

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

getOrElse works here too but doesn't matter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will swith back to getOrElse

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Hi, @zhengruifeng and @srowen (also cc @gengliangwang )

If you don't mind, could you hold on this refactoring a little bit more until @MaxGekk releases RC1?

  1. Currently, Apache Spark 3.3 is under active testing and unstable.2.
  2. GitHub Action PR builder also doesn't provide you the ANSI test result
  3. This kind of massive refactoring increases the complexity during the Apache Spark 3.3.0 release process.

Hopefully, please give us more time until we finish Apache Spark 3.3, @zhengruifeng . I believe Cleanups and refactoring PRs are less urgent than the scheduled release.

@zhengruifeng
Copy link
Contributor Author

@dongjoon-hyun Ok, I will hold on this PR since its target version is 3.4

@dongjoon-hyun
Copy link
Member

Thank you so much!

@srowen
Copy link
Member

srowen commented Jun 17, 2022

We can revisit this now. Rerun tests I think?

@dongjoon-hyun
Copy link
Member

+1 for restarting. Thank you, @zhengruifeng and @srowen .

@zhengruifeng
Copy link
Contributor Author

Sure, let me update this PR

@github-actions github-actions bot added the BUILD label Jun 18, 2022
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @zhengruifeng , @srowen , @huaxingao .
Merged to master.

@zhengruifeng
Copy link
Contributor Author

Thanks all!

@zhengruifeng zhengruifeng deleted the validate_cleanup branch June 19, 2022 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants