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.
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
Ibis check backend #1831
Ibis check backend #1831
Changes from 3 commits
4d35817
b509e5b
9cb21bf
705176a
1a18bf5
7afcc40
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nit: pandera/engines/ibis_engine.py defines
IbisObject = Union[ir.Column, ir.Table]
; should that just import this instead, or could that set of types be different in some situation?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.
would be the "standard" Ibis import for this.
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.
Will need to
import ibis.selectors as s
above, but then you can:More like the Polars implementation.
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.
thanks! was scouring the ibis API for this but missed the
across
selector.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.
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 necessary? If so, what's the ideal behavior (for my understanding)?
I.e. would you like to (automatically?) cast to boolean if the check doesn't return a boolean, or should we explicitly make sure each of the columns is a boolean?
Regardless, we can ignore the
idt
import altogether by checkingdtype.is_boolean()
.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.
No, this shouldn't be automatic... if a check function doesn't return a boolean it's a user/developer error.
The expectation of a check function is it should produce a scalar boolean, column boolean, or table boolean, since it's meant to produce a set of truth values about data.
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.
seems simpler (and more like the logic in the Polars backend)
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.
Similar to comment for
postprocess(..., ir.Table)
below, I thinkcheck_output
will need to have the original input as well as check results for this to work reliably.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 don't think row order can be guaranteed like this.
I think the way to do this would be to create a set of result columns in
apply()
instead of overwriting the data columns.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.
Can you suggest a code change for this? I was racking my brain on how to basically get the failure cases from the original data based on the check output boolean values.
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.
Yeah; let me make a PR to your branch, probably easier since it'll span a couple functions. The output of
apply()
is not used raw (without postprocessing) anywhere, right?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.
@cosmicBboy #1855 does this, with one caveat: instead of returning a table (in the case of a check function that takes a table and returns a table), I changed the API to return a dictionary of column names to expressions, because returning a table creates something that cannot be aligned with the original data.
This makes sense to me, but I wanted to see what you think.
(I also made most of the other changes suggested here in that PR.)
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.
By the way, the reason things work is because the tests are using the pandas backend. Now that I think about it, we should test with DuckDB or something else (could even be Polars, I suppose), because Ibis will drop the pandas backend. This will also provide a bit better guardrails around doing stuff that only works in pandas.