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

[SW-2639] Expose Fields of Model Output on H2OMOJOModel Classes as Getters #2692

Merged
merged 9 commits into from
Jan 4, 2022

Conversation

mn-mikke
Copy link
Collaborator

@mn-mikke mn-mikke commented Dec 15, 2021

Copy link
Member

@krasinski krasinski left a comment

Choose a reason for hiding this comment

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

Looks great! 👍


val explicitFieldImplementations = parameterSubstitutionContext.explicitFields.flatMap(_.mojoImplementation) ++
parameterSubstitutionContext.deprecatedFields.flatMap(_.mojoImplementation)

val imports = Seq(
"scala.collection.JavaConverters._",
"com.google.gson.JsonObject",
Copy link
Member

Choose a reason for hiding this comment

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

btw. not directly related to that PR but when I was doing some changes in that class I was thinking if there's a reason we use GSON here and not some Scala specific library as it's a bit hard to work with sometimes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

H2O-3 already uses GSON for json parsing. (see https://github.com/h2oai/h2o-3/blob/879e58d3e5f98064b738189febecd845bf4f87c4/h2o-genmodel/src/main/java/hex/genmodel/ModelMojoReader.java). I think the main motivation why we didn't go for another lib was that GSON is already on driver and executor classpath.

Copy link
Member

Choose a reason for hiding this comment

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

that's what I thought :) maybe (just maybe) we could consider json4s in the future if we ever have time for improvements as I see spark uses that

val h2oName = output.h2oName
val swName = output.swName
val value = output.dataType.getSimpleName match {
case "boolean" => s"""outputSection.get("$h2oName").getAsBoolean()"""
Copy link
Member

Choose a reason for hiding this comment

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

can the outputSection... prefix be extracted here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

outputSection is not a prefix in all cases (see case "TwoDimTableV3")

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.

None yet

3 participants