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

Allow copying model instances #270

Merged
merged 3 commits into from
Apr 8, 2019
Merged

Allow copying model instances #270

merged 3 commits into from
Apr 8, 2019

Conversation

tovbinm
Copy link
Collaborator

@tovbinm tovbinm commented Apr 8, 2019

Related issues
Currently once the model is loaded it's impossible to reuse the same instance across multiple threads.

Describe the proposed solution
Added model.copy() to allow deep copy of model instance so each thread would get it's own instance.

val copy =
new OpWorkflowModel(uid = uid, trainingParams = trainingParams.copy())
.setFeatures(copyFeatures(resultFeatures))
.setRawFeatures(copyFeatures(rawFeatures))
Copy link
Collaborator

Choose a reason for hiding this comment

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

you shouldnt set the raw features - they should be derived from the features

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point. what if somebody modifies it separately? I dont want to mess up any internal state.

Copy link
Collaborator

Choose a reason for hiding this comment

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

they shouldnt be able to - it is private to op. setting the raw features should never be done directly. In fact the method has no usages maybe we shoudl delete it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ha ha ha!! Removed it.

@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #270 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
+ Coverage   86.38%   86.39%   +<.01%     
==========================================
  Files         318      318              
  Lines       10447    10458      +11     
  Branches      321      559     +238     
==========================================
+ Hits         9025     9035      +10     
- Misses       1422     1423       +1
Impacted Files Coverage Δ
.../main/scala/com/salesforce/op/OpWorkflowCore.scala 95.16% <ø> (+1.51%) ⬆️
...main/scala/com/salesforce/op/OpWorkflowModel.scala 93.9% <100%> (+1.04%) ⬆️
...es/src/main/scala/com/salesforce/op/OpParams.scala 85.71% <0%> (-4.09%) ⬇️

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 3a1a2d8...90da387. Read the comment docs.

@tovbinm tovbinm merged commit b652257 into master Apr 8, 2019
@tovbinm tovbinm deleted the mt/model-copy branch April 8, 2019 23:38
@tovbinm tovbinm mentioned this pull request Apr 10, 2019
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.

2 participants