-
Notifications
You must be signed in to change notification settings - Fork 394
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
Avoid creating SparseVectors for LOCO #377
Merged
leahmcguire
merged 19 commits into
salesforce:master
from
gerashegalov:gera/perf-regression
Aug 21, 2019
Merged
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
66a5e99
WIP
gerashegalov 77b97ee
code check fixes
gerashegalov 35b8b5c
try sparse vector clone for test fix
gerashegalov cb2dc05
Merge branch 'master' into gera/perf-regression
michaelweilsalesforce a494129
WIP
gerashegalov 1c383e5
code check fixes
gerashegalov a1d7d81
try sparse vector clone for test fix
gerashegalov f95f4bf
better names
gerashegalov a8ff84f
Skip Zeros when computing LOCOs for Text and Date fields
mweilsalesforce 709594f
Merge branch 'gera/perf-regression' of github.com:gerashegalov/Transm…
mweilsalesforce 96b2941
Fix Scalastyle
mweilsalesforce ee76053
minor fixes
gerashegalov 2248d49
zipped
gerashegalov c3f743d
redundant toArray
gerashegalov dc39cb3
Updating comments + adding extra checks
mweilsalesforce 25a6ac5
Merge branch 'master' into gera/perf-regression
leahmcguire 3b7b63a
Adding TODO
mweilsalesforce b271b74
Merge branch 'master' into gera/perf-regression
tovbinm 087b1d2
Merge branch 'master' into gera/perf-regression
leahmcguire File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
code check fixes
- Loading branch information
commit 1c383e5390074d7d3069423f76aef9ea79b692b0
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
So the sparse features you just put in a value of 0? Cant we just skip adding them to the heap?
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 had the same idea but in one of the iteration I ran into test failures and deferred it to later. I'll recheck now that I have everything green. @michaelweilsalesforce any thoughts?
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.
What kind of failures have you encountered?
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.
it may be that we were doing an unnecessary calculation and that just happened to be captured in the test...
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.
@michaelweilsalesforce you can reproduce it by commenting out the line 171-172.
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.
@sanmitra
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.
@leahmcguire @gerashegalov The reason for tracking zero values is whenever we want to average LOCOs of a same raw text feature we are also including the zero values.
E.g if text feature
TextA
has on a row 6 non zero valuesloco1
, ...,loco6
and 4 0s, we are dividing by 10 :(
loco1
+loco2
+ ... +loco6
)/10There 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.
LEt me write a fix that will not go over the zeros