-
Notifications
You must be signed in to change notification settings - Fork 575
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
Remove the pad argument from amplitude embedding template #1805
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #1805 +/- ##
==========================================
- Coverage 98.92% 98.92% -0.01%
==========================================
Files 209 209
Lines 15733 15727 -6
==========================================
- Hits 15564 15558 -6
Misses 169 169
Continue to review full report at Codecov.
|
Hi @Jaybsoni, just a fly-by comment: don't forget to add an item to the Breaking Changes section about this. 🙂 (Users who've used Also, at a first glance, it will likely break |
@antalszava, Thanks for the reminder! I will update the change log! I can also update the demo! |
…nnylane into deprecate_pad_arg
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 looks good to me, I don't why coverage is complaining. Can you check it locally?
Local coverage report: Not sure why the overall coverage has decreased since the coverage for the amplitude.py file increased. I think this is good to merge? @josh146, @antalszava any ideas on what could be missing or can we just merge it? |
@Jaybsoni Normally it is just because more lines were deleted than added and CodeCoverage does not like that! |
I see! Well in that case I think its good to go. Let me know if there is anything else you think needs to be changed |
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 looks good to me :100 @josh146 In my opinion you can bypass the coverage check
We should probably read through the codecov documentation, maybe there is a setting we can change in our codecov configuration file to avoid this from happening 🤔 |
Deprecating the
pad
argument inAmplitudeEmbedding
for the next release.