Fix ibis bokeh datetime axis

[MarcSkovMadsen]

Closes #5521

This is my first real attempt at a contribution to HoloViews. So please be nice to me 😄

I hope an experienced core contributor can show me how this should really be solved or point me in the right direction. It a bit hard to do a real unit test because the functions contains so much code.

[codecov-commenter]

Codecov Report

Merging #5522 (d66dd2e) into master (0317237) will decrease coverage by 0.50%.
The diff coverage is 12.28%.

@@            Coverage Diff             @@
##           master    #5522      +/-   ##
==========================================
- Coverage   88.07%   87.57%   -0.51%     
==========================================
  Files         302      302              
  Lines       62283    62335      +52     
==========================================
- Hits        54855    54589     -266     
- Misses       7428     7746     +318     

Impacted Files	Coverage Δ
holoviews/core/util.py	85.91% <ø> (ø)
holoviews/tests/core/data/test_ibisinterface.py	4.80% <10.63%> (-75.69%)	⬇️
holoviews/core/data/ibis.py	31.14% <20.00%> (-48.32%)	⬇️
holoviews/core/data/util.py	53.57% <0.00%> (-5.36%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[MarcSkovMadsen]

I could see that after my change a CategoricalAxis would not be correctly identified.

I have tried for 1½ hours now to fix, but the function _axes_props is doing way too much and there is no documentation. I don't have a chance to fix. I give up.

[MarcSkovMadsen]

Ok. Wasted another hour. I can that this function does not work for the ibis backend either.

And it is so long and complicated. And its even changing the specified parameters ranges in place. Makes it quite hard to reason about.

[hoxbro]

I think your previous implementation was better. This will be slow if the database table has a billion rows.

[MarcSkovMadsen]

The problem is that the previous implementation did not produce the expected results.

[hoxbro]

I think this is a problem with ibis. I will probably add an if statement to your previous implementation, checking if the output is a pd.series and take the first value if it is.

import os
import sqlite3

import ibis
import pandas as pd


def create_sqlite(df, name):
    filename = "tmp.sqlite"
    os.remove(filename)
    con = sqlite3.Connection(filename)
    df.to_sql(name, con, index=False)
    return ibis.sqlite.connect(filename).table(name)


df = pd.DataFrame(
    {"string": ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"]},
)
sqlite_table = create_sqlite(df, "table")
pandas_table = ibis.pandas.connect({"df": df}).table("df")

pandas_table["string"], sqlite_table["string"]
pandas_table["string"].first().execute()
sqlite_table["string"].first().execute()

[philippjfr]

Is there a better way to figure out that we are working with a datetime column? This approach for getting the first and last value really should be removed entirely. Indeed at some point I planned to consistently return None for all non-numeric and non-datetime types.

[hoxbro]

This is not for a datetime column but for a categorical column.

[hoxbro]

@MarcSkovMadsen can correct me. But the fix for the datetime column was done by adding dimension_type to the class.

[philippjfr]

Just to fully explain, range is really an operation that should be reserved for numeric and datetime values. Early on I decided that in the categorical/string/object case I would simply grab the first/last value in the array. This was fine when the only interfaces we had were in-memory but for lazy interfaces like SQL or dask backed data the behavior is often ill-defined or actually quite expensive to compute. So the correct thing going forward is to simply abort early and return (None, None) if the column is categorical. The remaining problem is that some interfaces do not correctly declare datetime columns, reporting an object dtype. So we will still need to handle that case correctly.

[MarcSkovMadsen]

I simply don't understand what I should do here to resolve

[philippjfr]

That's understandable, I was just trying to provide context and didn't have any prescription for what should be done.

My question is whether the change to dimension_type is sufficient to fix the issue? If so I would suggest reverting the change to range and eventually removing this conditional branch entirely.

[MarcSkovMadsen]

Its not enough. Both the DateTime and Categorical axis where broken. Needs two different fixes