-
Notifications
You must be signed in to change notification settings - Fork 8
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 Remainder, still need the tests #27
Conversation
Codecov Report
@@ Coverage Diff @@
## master #27 +/- ##
==========================================
- Coverage 77.50% 73.41% -4.10%
==========================================
Files 12 12
Lines 289 331 +42
==========================================
+ Hits 224 243 +19
- Misses 65 88 +23
Continue to review full report at Codecov.
|
I was writing the tests but forgot that the other PR will make changes in tests/transforms.jl, so I decided not to push the tests for the Remainder for now. |
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've finished adjusting small details. Now we need two more adjustments:
- The column :remainder should be called
total_minus_abcdef...
whereabcdef
is just a concatenation of names in the variablenames
. - The two function
apply
andreapply
only differ by the definition oftotal = func(transform, table, cache)
. Inapply
the function computes the total and inreapply
the function reads the cache. Can you try to refactor the code that way so that we reduce the amount of code repetition?
Closing in favor of #28 |
Trying to add Remainder transformation, but the tests is needed