Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[SPARK-30820][SPARKR][ML] Add FMClassifier to SparkR #27570
[SPARK-30820][SPARKR][ML] Add FMClassifier to SparkR #27570
Changes from 8 commits
d44582f
a018ce9
2d6af0d
7c5fcf0
a41ae58
c16e8be
607b66d
6e56263
7126bbf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you checked this TODO yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it more for a discussion. Adding custom formula components is not very hard, the question is if it makes sense to complicate for such thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also check the predict result here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if such check are really useful here. In practice fitting is not unlikely failure point and most likely problems are related to parameter passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked other classification tests. It seems other tests checked the
typeof
and result of the prediction. I guess it might be better to be consistent with other tests?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeof
is not applicable here.typeof
is S compatibility thingy, and can be used only to distinguish between core types (here it could only determine if value is S4 type).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me that all the other ML R tests check the prediction result. For example, in LinearSVM,
Is it OK if we do something similar here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure we can. The question is what we are really trying to test in such cases? What types of implementation mistakes can we detect here, that are not already covered by JVM tests and / or SparkR data frames tests?
These checks involve additional jobs and many tests are already rejected to keep things manageable, so unless these serve specific purpose, I'd prefer to keep things lean here.
In contrast there are many SparkR ML failure modes that are real, and could be tested, but are crippled by lack of required API. But that's way beyond the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am OK with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used to avoid failures in case of missing
winutils
. If i recall correctly the primary target was CRAN tests (and these shouldn't run here anyway), but I think it still applicable to AppVeyor.