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

Serialization for FeatureGeneratorStage #300

Closed

Conversation

wsuchy
Copy link
Contributor

@wsuchy wsuchy commented Apr 20, 2019

Only saving to JSON

@wsuchy wsuchy requested a review from tovbinm April 20, 2019 02:51
@wsuchy wsuchy requested a review from leahmcguire as a code owner April 20, 2019 02:51
@codecov
Copy link

codecov bot commented Apr 20, 2019

Codecov Report

Merging #300 into mt/function-arguments-merged will decrease coverage by 0.38%.
The diff coverage is 43.18%.

Impacted file tree graph

@@                       Coverage Diff                        @@
##           mt/function-arguments-merged     #300      +/-   ##
================================================================
- Coverage                         86.33%   85.94%   -0.39%     
================================================================
  Files                               325      325              
  Lines                             10511    10576      +65     
  Branches                            338      548     +210     
================================================================
+ Hits                               9075     9090      +15     
- Misses                             1436     1486      +50
Impacted Files Coverage Δ
...lesforce/op/utils/reflection/ReflectionUtils.scala 84.09% <0%> (-13.28%) ⬇️
...ce/op/stages/OpPipelineStageJsonReaderWriter.scala 71.11% <100%> (ø) ⬆️
...cala/com/salesforce/op/OpWorkflowModelWriter.scala 100% <100%> (ø) ⬆️
...la/com/salesforce/op/features/FeatureBuilder.scala 33.33% <100%> (-0.94%) ⬇️
...cala/com/salesforce/op/OpWorkflowModelReader.scala 94.64% <100%> (+0.19%) ⬆️
...m/salesforce/op/stages/FeatureGeneratorStage.scala 23.52% <2.77%> (-49.81%) ⬇️
...com/salesforce/op/test/PassengerFeaturesTest.scala 90.32% <88.46%> (-9.68%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87f87fc...05a4232. Read the comment docs.

@@ -138,6 +138,8 @@ class OpWorkflowModelReader(val workflowOpt: Option[OpWorkflow]) extends MLReade
val originalStage = workflow.stages.find(_.uid == stageUid)
originalStage match {
case Some(os) => new OpPipelineStageReader(os).loadFromJson(j, path = path).asInstanceOf[OPStage]
case None if stageUid.startsWith("FeatureGeneratorStage_") =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

classOf[FeatureGeneratorStage].getSimpleName instead of string

aggregator = new CustomMonoidAggregator[O](associativeFn = fn, zero = zero)(tto)
this
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

why are these deleted? they have an important function

@@ -181,6 +181,9 @@ class FeatureBuilderTest extends FlatSpec with TestSparkContext {

}

object TestCustomMonoidAggregator extends CustomMonoidAggregator[Real](zero = Real.empty.v,
Copy link
Collaborator

Choose a reason for hiding this comment

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

so people need to instantiate the class for the aggregation function?

object TestMonoidAggregator
extends CustomMonoidAggregator[Real](None, (l, r) => (l -> r).map(breeze.linalg.max(_, _))) with Serializable

object PassengerFeaturesTestLambdas {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you need the functions defined this way or is it a style thing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

that the new thing

@tovbinm
Copy link
Collaborator

tovbinm commented May 3, 2019

@leahmcguire it's WIP, not for review yet ;)

@tovbinm tovbinm closed this May 10, 2019
@tovbinm tovbinm deleted the ks/feature-generator-stage-serde branch May 10, 2019 19:54
@tovbinm
Copy link
Collaborator

tovbinm commented May 10, 2019

Superseeded by #309

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