-
Notifications
You must be signed in to change notification settings - Fork 100
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
improve datachain subtract #352
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #352 +/- ##
==========================================
+ Coverage 86.69% 86.72% +0.02%
==========================================
Files 91 91
Lines 10075 10092 +17
Branches 2042 2051 +9
==========================================
+ Hits 8735 8752 +17
Misses 985 985
Partials 355 355
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
1b3468e
to
171818b
Compare
41fc4a7
to
caccaf4
Compare
src/datachain/query/dataset.py
Outdated
@@ -296,15 +296,23 @@ def q(*columns): | |||
|
|||
@frozen | |||
class Subtract(DatasetDiffOperation): | |||
on: Sequence[str] | |||
on: Sequence[Union[str, tuple[str, str]]] |
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.
Shouldn't this be Sequence[tuple[str, str]]
?
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.
Shouldn't this be
Sequence[tuple[str, str]]
?
Yeah.I will update it.Thank you.
tests/unit/lib/test_datachain.py
Outdated
@@ -1811,3 +1846,32 @@ def test_from_csv_nan_inf(tmp_dir, test_session): | |||
assert np.isnan(res[0]) | |||
assert np.isposinf(res[1]) | |||
assert np.isneginf(res[2]) | |||
|
|||
|
|||
def test_subtract_with_different_column_names(test_session): |
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.
Is this test useful? It seems to duplicate the addition to test_subtract()
.
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.
Is this test useful? It seems to duplicate the addition to
test_subtract()
.
Sorry.I forgot to delete it and will do so 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.
LGTM now, thank you!
c260d66
to
ccf4375
Compare
Fix: DataChain.subtract() on differently named columns
Fixes #181