Skip to content
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 .hvplot for ibis #990

Merged
merged 2 commits into from Dec 1, 2022
Merged

fix .hvplot for ibis #990

merged 2 commits into from Dec 1, 2022

Conversation

MarcSkovMadsen
Copy link
Collaborator

@MarcSkovMadsen MarcSkovMadsen commented Nov 26, 2022

Closes #988

After this change I can produce a plot from the example in #988. But it seems it does not understand the Timestamp type should be shown as datetime on the x-axis. I will create a separate issue with holoviews for that.

image

I've created an issue here holoviz/holoviews#5521.

This was referenced Nov 27, 2022
@MarcSkovMadsen MarcSkovMadsen changed the title fix ibis fix .hvplot for ibis Dec 1, 2022
Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@philippjfr
Copy link
Member

Really appreciate you looking into this. The ibis backend was left in a half-finished state when the folks working on it ran out of funding. So reviving it is pretty exciting and if we can make it work well let's write a blog post about it.

@MarcSkovMadsen
Copy link
Collaborator Author

Thanks. Does that mean you will now approve and merge @philippjfr ?

@philippjfr
Copy link
Member

Was giving Maxime a chance to review but I'll just go ahead and merge now.

@philippjfr philippjfr merged commit f48e716 into master Dec 1, 2022
@philippjfr philippjfr deleted the fix/ibis branch December 1, 2022 20:09
@maximlt
Copy link
Member

maximlt commented Dec 2, 2022

Sorry it's been a short and busy week.

Thanks Marc for working on that. I will just say that personally I don't consider that as a priority as Ibis was never advertised in hvPlot, my focus is rather on stability/bug fixes and docs.

A couple of comments on that specific PR:

  • its title isn't very explicit, it looks like everything has been fixed :)
  • the if statement should have a comment that explains why hasattr(data.columns, 'name') is required (I do not know why ibis doesn't have the same interface as Pandas'). The general lack of comments make the converter difficult to apprehend.
  • In the test file the try/except block should try to catch ImportError
  • In the test file importing pandas shouldn't be in the try/except block as pandas is a dependency of hvPlot so ensured to be always there when the test suite is executed
  • ibis is not installed so these new tests will never run. HoloViews does install ibis part to run its test suite. It actually was a bit of a pain to get it to installed with conda, that wouldn't solve for ibis-framework (Cannot install on Linux conda-forge/ibis-framework-feedstock#54) so we mapped it to ibis-sqlite for conda but kept ibis-framework for PyPI. Maybe this can be revisited now.

@maximlt
Copy link
Member

maximlt commented Dec 2, 2022

@MarcSkovMadsen which version of ibis did you you? How did you install it?

@MarcSkovMadsen
Copy link
Collaborator Author

pip install ibis-framework and pip install ibis-framework[duckdb]. Version 3.2.0.

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Dec 2, 2022

Thanks. Should I update try/ except across the files? I saw what was in other files and copied that.

image

This is another example

image

Those wrong imports are all over the place in the code.

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Dec 2, 2022

Regarding the title. When I wrote it, I thought HoloViews was supporting Ibis well and this was just a small thing and then ibis would work well with hvplot. Now I can see that is far from the situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ibis backend not working: 'list' object has no attribute 'name'
3 participants