[FEATURE] - DataTable Formatters #13685
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## branch-3.4 #13685 +/- ##
==============================================
- Coverage 92.59% 92.58% -0.01%
==============================================
Files 323 325 +2
Lines 20575 20728 +153
==============================================
+ Hits 19051 19191 +140
- Misses 1524 1537 +13 |
|
@thomktz I've left a few comments, I've also added @mattpap for review since he is most heavily involved in BokehJS development these days. He may have some suggestions and guidance for adding other kinds of tests, or details of the implementation. Some other things that might also be nice, depending on your availability to devote more time:
We can make issues for follow-on if you don't have bandwidth for these. Lastly @thomktz would you like to be added to @bokeh/dev ? This is really a terrific first PR for diving in to BokehJS, and certainly reason to add you in case you'd are able to continue being involved. This is going to be a great feature to show off in a release blog! |
…ize limit for bokeh-tables.min.js
|
I do have some time to make thorough examples, docs and tests for this, with guidance. Regarding examples, I have modified these two: integration/widgets/data_table_customization.py old/new interaction/tools/point_draw.py old/new |
| resolvedFontStyle = font_style.value | ||
| } else if (isField(font_style)) { | ||
| resolvedFontStyle = _dataContext[font_style.field] | ||
| } |
There was a problem hiding this comment.
This is unfortunate, because this should be deferred to vectorized properties to figure out how to resolve this. Especially to handle ndarrays properly, among other issues. Having access to the data source, we could write something like this:
this.properties.font_style.uniform(data_source).get(index)though this would need to be precomputed, perhaps as some kind of virtual column in table's data provider.
There was a problem hiding this comment.
I will probably need guidance on this - not sure where/when to precompute. Are we doing something similar somewhere else in the code I can use for inspiration ?
There was a problem hiding this comment.
@mattpap is this something we could merge as-is for now with a follow-up issue to improve later?
There was a problem hiding this comment.
This is definitely not ideal and compiles only because it's mostly dynamically typed code, but given the proper solution is non-trivial and that this isn't critical code path in bokehjs, then yes we can proceed with this and improve later.
| } else if (isField(background_color)) { | ||
| if (background_color.transform != null && background_color.transform instanceof ColorMapper) { | ||
| const rgbaArray = background_color.transform.rgba_mapper.v_compute([_dataContext[background_color.field]]) | ||
| const [r, g, b, a] = rgbaArray |
There was a problem hiding this comment.
This is a bit confusing at the first glance, but given that v_compute returns a flat array (technically an ndarray), then this is indeed correct. Sadly there is no scalar variant in ColorMapper, so we will have to keep this as-is.
|
Thanks @thomktz ! 🎉 |
* Revamp StringFormatter * Remove dev script * fix examples * isort * No need for 'normal' style * Use existing Specs * Working colormaps * Increase size from 350 to 450 * Ruff & isort * Update import path for ColorMapper in cell_formatters.ts and adjust size limit for bokeh-tables.min.js * point_draw and data_table_customization examples revamp * isort * Add luminance, use isExpr * ESLint * Add test for luminance calculation in RGB class * Update luminance test precision * Remove extra variables * PR comments
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Still have to discuss whether this approach is worth finishing, and then make the other Formatters compatible.