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

Add the CSV hash table in Hash layer and fix a bug. #385

Merged
merged 14 commits into from Jul 3, 2021
Merged

Conversation

dengc367
Copy link
Collaborator

  • delete the Lambda sublayer in LocalActivationUnit Layer class

  • add vocabulary_path in the SparseFeat to support the csv HashTable functionality

  • update docs and add examples in doc

  • Remove trailing whitespace

@dengc367
Copy link
Collaborator Author

this PR will be merged into release branch, and so you need merge into master branch when this PR is merged. @shenweichen

@codecov
Copy link

codecov bot commented Jun 29, 2021

Codecov Report

Merging #385 (8804e7e) into release (0df401c) will increase coverage by 0.32%.
The diff coverage is 95.45%.

Impacted file tree graph

@@             Coverage Diff             @@
##           release     #385      +/-   ##
===========================================
+ Coverage    94.66%   94.99%   +0.32%     
===========================================
  Files           54       54              
  Lines         2924     2935      +11     
===========================================
+ Hits          2768     2788      +20     
+ Misses         156      147       -9     
Flag Coverage Δ
pytest 94.99% <95.45%> (+0.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
deepctr/inputs.py 95.83% <66.66%> (ø)
deepctr/feature_column.py 92.92% <100.00%> (+0.12%) ⬆️
deepctr/layers/core.py 99.09% <100.00%> (-0.01%) ⬇️
deepctr/layers/sequence.py 90.58% <100.00%> (+0.47%) ⬆️
deepctr/layers/utils.py 91.42% <100.00%> (+6.04%) ⬆️

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 0df401c...8804e7e. Read the comment docs.

@shenweichen shenweichen deleted the branch release June 29, 2021 08:10
@shenweichen shenweichen reopened this Jun 29, 2021
@dengc367
Copy link
Collaborator Author

I added some pytest code of hash vocabulary_path parameter

@dengc367
Copy link
Collaborator Author

dengc367 commented Jul 2, 2021

@shenweichen I used your layer_test function in pytest, and when that run the line model.predict and throws the exception Failed precondition: Table not initialized., I think the pytest environment is the tfv1.

@dengc367
Copy link
Collaborator Author

dengc367 commented Jul 2, 2021

It's strange, I have tested successfully in my local linux, but it always failed when using CI action with exception: Failed precondition: Table not initialized..
image

@shenweichen
Copy link
Owner

@shenweichen I used your layer_test function in pytest, and when that run the line model.predict and throws the exception Failed precondition: Table not initialized., I think the pytest environment is the tfv1.

i think you need to test it in your local machine in both tf1 and tf2

@dengc367
Copy link
Collaborator Author

dengc367 commented Jul 2, 2021

I have modified some code to fix a bug An op outside of the function building code is being passed a "Graph" tensor in PostionalEncoding Layer, which substituted the tf.concat with np.zeros when zero_pad=True, and remove the tf.compat.v1.disable_eager_execution() in sequence_test.py, so all the unit tests in test/layers directory are successful.

@dengc367 dengc367 merged commit 510ecf0 into release Jul 3, 2021
shenweichen added a commit that referenced this pull request Jul 5, 2021
@shenweichen
Copy link
Owner

committers are not allowed to merge their own branches to release, we will review your pull request soon.

@dengc367
Copy link
Collaborator Author

dengc367 commented Jul 5, 2021

OK. Thanks.

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

2 participants