-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
xvec support #1405
base: main
Are you sure you want to change the base?
xvec support #1405
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1405 +/- ##
==========================================
+ Coverage 87.39% 88.68% +1.28%
==========================================
Files 50 51 +1
Lines 7490 7509 +19
==========================================
+ Hits 6546 6659 +113
+ Misses 944 850 -94 ☔ View full report in Codecov by Sentry. |
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 simple enough to me and I'm okay with merging BUT as we keep adding data backends that simply convert I'd maybe suggest a plugin mechanism where you can register a converter function rather than adding a bunch of if/else cases.
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.
When we add support for a new data backend I'd like to see us explain more in depth why we want to add it. Things like what motivated the PR, how well established and maintained the backend is, how popular it is, etc. I personally don't know xvec
and neither this PR nor the GeoViews issue provides much information about it. It may have been mentioned in a HoloViz meeting but I also saw no trace of that looking at the notes.
FWIW it's contributed by the maintainers of xarray and geopandas It's quite new, so not very established, but because it's by maintainers of xarray/geopandas + part of earthmover (the cofounders started the Pangeo movement), I imagine it'll establish its name over time. The motivation can be found in the blog post
Also, unsure whether this should be a part of geoviews first before hvplot |
Thanks for the details. It looks indeed that it's very early stage in terms of adoption: I found this issue (xarray-contrib/xvec#82) on their repo where they're discussion plotting capabilities. I'd encourage you to chime in and see with them if we could easily provide solutions. If a collaboration gets established, there's more chance we'll be successful, i.e. the interface we build ends up actually being used by real users.
Also unsure, I imagine in hvPlot it should be integrated as a simple conversion layer while in GeoViews it'd be more involved. |
Closes holoviz/geoviews#737
Not entirely sure what the level of support should be implemented for xvec; should this be in geoviews or just hvplot?
In attempt 2 (current), I convert the xarray dataset into geopandas dataframe, "flattening" extra geometries, i.e. converting them into integer indices which is done through
drop_vars
, and then gathering all the xarray dims as groupby; that way, it still shows up as slider widgetsScreen.Recording.2024-08-29.at.5.08.31.PM.mov
However, the integers aren't meaningful so I was wondering if there are extra geometries, should I overlay centroid points of the other geometries? If so, how do I even do that in hvplot?
In attempt 1, I try to keep it in its xarray data structure, but requires much more change in hvplot to plot geometries nested in xarray.
cc: @hoxbro