feat: DH-18165: Add calendar argument to several dx charts #1122
Conversation
There was a problem hiding this comment.
PR Overview
This PR adds support for a calendar argument to several chart types in the dx charts library, along with updates to documentation and tests. Key changes include:
- Adding a calendar parameter to chart APIs and propagating the calendar option to underlying plotly figures.
- Updating documentation for OHLC, candlestick, scatter, line, and area charts to include calendar usage.
- Adjusting tests and internal implementation (e.g. in FigureCalendar, _private_utils, and PartitionManager) for calendar handling.
Reviewed Changes
| File | Description |
|---|---|
| plugins/plotly-express/src/deephaven/plot/express/deephaven_figure/FigureCalendar.py | Introduces the FigureCalendar class and methods to convert calendar data for the frontend. |
| plugins/plotly-express/docs/*.md | Updates documentation for various chart types to include calendar usage examples. |
| plugins/plotly-express/src/js/src/PlotlyExpressChartModel.test.ts | Updates tests to accommodate new calendar behavior and related events. |
| plugins/plotly-express/src/deephaven/plot/express/plots/*.py | Adds calendar parameters to chart plotting functions and updates logic to force svg render mode when a calendar is provided. |
| Other files | Adjustments in data generators, custom draw functions, and internal utils to support the new calendar functionality, plus minor grammar fixes. |
Copilot reviewed 37 out of 37 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
plugins/plotly-express/src/deephaven/plot/express/deephaven_figure/FigureCalendar.py:216
- Overriding dict as a method might cause confusion with the built-in dict attribute. Consider renaming this method to to_dict() to clearly indicate its purpose.
def __dict__(self) -> FigureCalendarDict | None:
| base_time = to_j_instant( | ||
| "2018-06-01T08:00:00 ET" | ||
| ) # day deephaven.io was registered | ||
| base_time = to_j_instant(starting_time) # day deephaven.io was registered |
There was a problem hiding this comment.
Comment doesn't make sense here anymore.
I'd declare a constant for the "2018-06-01..." date, have the comment with that constant definition, and then use that as the default for the method.
| this.updateLayout(getWidgetData(widget)); | ||
|
|
||
| // The calendar is only fetched once at init. | ||
| // Timezone must be replaced for accurate rangebreaks. | ||
| const { calendar } = getWidgetData(widget).figure.deephaven; |
There was a problem hiding this comment.
No need to parse the widget data twice. Just get the layout and then use it, e.g.
const widgetData = getWidgetData(widget);
this.updateLayout(widgetData);
this.updateCalendar(widgetData);| this.tableSubscriptionMap.get(tableId)?.close(); | ||
| this.tableSubscriptionMap.delete(tableId); |
There was a problem hiding this comment.
Should factor this into an unsubscribeTable method, so it mirrors the call to subscribeTable.
Then this would just be:
this.unsubscribeTable(tableId);
this.subscribeTable(tableId);
| val => | ||
| this.chartUtils.unwrapValue( | ||
| val, | ||
| ( | ||
| this.formatter?.getColumnTypeFormatter( | ||
| 'datetime' | ||
| ) as DateTimeColumnFormatter | ||
| ).dhTimeZone | ||
| ), |
There was a problem hiding this comment.
This has poor performance - we're re-fetching the column type formatter for every value.
See how we memoize the value translator/fetch functions in web-client-ui: https://github.com/deephaven/web-client-ui/blob/3f74d4403f332fa65877bc33e418faea3fc974c6/packages/chart/src/FigureChartModel.ts#L411
| this.calendar = { | ||
| ...calendar, | ||
| timeZone, | ||
| } as unknown as DhType.calendar.BusinessCalendar; |
There was a problem hiding this comment.
Hmmm this isn't quite right... you need to cast it, because the holiday.date is just a string instead of the LocalDateWrapper type that BusinessCalendar has... And this is currently working because we just call holiday.date.toString() in ChartUtils.
At the very least there needs to be some comments about why that's being done. But I'd rather wrap the string in an object that does match the interface correctly, so converting those holiday.dates to something that conforms to:
export interface LocalDateWrapper {
valueOf():string;
getYear():number;
getMonthValue():number;
getDayOfMonth():number;
toString():string;
}
Otherwise we open ourselves up to issues later.
|
plotly-express docs preview (Available for 14 days) |
Fixes DH-18165
Adds calendar option to several chart types. Some good examples are in the docs and e2e tests.
I also noticed some of the js tests were not quite correct when making my new ones. Since there was just a few straightforward changes I made them in this ticket.