- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 407
Fix ibis duckdb no rowid #5527
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
Fix ibis duckdb no rowid #5527
Conversation
| def test_datetime_xaxis(self): | ||
| """Test to make sure a DateTimeAxis can be identified for the bokeh backend""" | ||
| plot_ibis = Curve(self.reference_table, kdims="date", vdims="actual") | ||
| # When | ||
| plot_bokeh = render(plot_ibis, "bokeh") | ||
| xaxis, yaxis = plot_bokeh.axis | ||
| # Then | ||
| assert isinstance(xaxis, bokeh_axes.DatetimeAxis) | ||
| assert isinstance(yaxis, bokeh_axes.LinearAxis) | ||
|  | ||
| def test_categorical_xaxis(self): | ||
| """Test to make sure a Categorical axis can be identified for the bokeh backend""" | ||
| plot_ibis = Curve(self.reference_table, kdims="string", vdims="actual") | ||
| # When | ||
| plot_bokeh = render(plot_ibis, "bokeh") | ||
| xaxis, yaxis = plot_bokeh.axis | ||
| # Then | ||
| assert isinstance(xaxis, bokeh_axes.CategoricalAxis) | ||
| assert isinstance(yaxis, bokeh_axes.LinearAxis) | ||
|  | ||
| def test_numerical_xaxis(self): | ||
| """Test to make sure a LinearAxis axis can be identified for the bokeh backend""" | ||
| plot_ibis = Curve(self.reference_table, kdims="numerical", vdims="actual") | ||
| # When | ||
| plot_bokeh = render(plot_ibis, "bokeh") | ||
| xaxis, yaxis = plot_bokeh.axis | ||
| # Then | ||
| assert isinstance(xaxis, bokeh_axes.LinearAxis) | ||
| assert isinstance(yaxis, bokeh_axes.LinearAxis) | 
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.
First of all, really appreciate your work investigating these issues. That said these tests can't live here. The plotting and data parts of HoloViews are separated for a reason and if test coverage of the data parts is sufficiently extensive then there's no reason to ever test the plotting code with different data backends.
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.
if test coverage of the data parts is sufficiently extensive
To be clear, that is of course clearly not the case since you're discovering these issues.
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.
Besides the non-comprehensive test, the missing docstrings and type annotations from the base Interface also makes it harder than it has to be, to know what a method on the interface actually should do and return.
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.
How would someone be able to setup appropriate tests for a backend @philippjfr? If I look into the tests/core/data folder I see nothing that systematically tests the same things across backends?
And knowing what to test for is also quite difficult due to missing docstrings, type annotations, very complex functions and similar.
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 are various baseclasses in tests/core/data which implement systematic tests which are then subclassed for each interface.
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.
The best thing to do is probably to continue extending tests/core/data/base HeterogeneousColumnTests with more tests.
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.
Specifically testing for datetime columns seems to be pretty lacking.
| Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@            Coverage Diff             @@
##             main    #5527      +/-   ##
==========================================
- Coverage   88.07%   87.56%   -0.51%     
==========================================
  Files         302      302              
  Lines       62283    62338      +55     
==========================================
- Hits        54855    54589     -266     
- Misses       7428     7749     +321     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
…x/ibis-duckdb-no-rowid
…x/ibis-duckdb-no-rowid
| Will be fixed by #6718.   from pathlib import Path
import duckdb
import holoviews as hv
import hvplot.ibis
import hvplot.pandas
import ibis
import pandas as pd
hvplot.extension("plotly")
DUCKDB_PATH = "DuckDB.db"
pandas_df = pd.DataFrame(
    {
        "actual": [100, 150, 125, 140, 145, 135, 123],
        "forecast": [90, 160, 125, 150, 141, 141, 120],
        "numerical": [1.1, 1.9, 3.2, 3.8, 4.3, 5.0, 5.5],
        "date": pd.date_range("2022-01-03", "2022-01-09"),
        "string": ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"],
    },
)
if not Path(DUCKDB_PATH).exists():
    duckdb_con = duckdb.connect(DUCKDB_PATH)
    duckdb_con.execute("CREATE TABLE df AS SELECT * FROM pandas_df")
ibis.options.sql.default_limit = None
db = ibis.duckdb.connect(DUCKDB_PATH)
table = db.table("df")
hv.Bars(table, kdims="string", vdims="actual").redim.values(
    string=["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"]
) | 
Closing #5526
Depends on #5522