-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: indicator chart #1088
base: main
Are you sure you want to change the base?
feat: indicator chart #1088
Conversation
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.
Can we also add an e2e test in tests/app.d/express.py
?
@@ -262,6 +304,7 @@ indicator_plot = dx.indicator( | |||
value="Price", | |||
reference="StartingPrice", | |||
by="Sym", | |||
by_vars=("increasing_color", "decreasing_color"), |
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.
This example is kind of confusing. What is the by_vars
argument for? Why are we doing a by
here at all when we've already filtered the table to one Sym
?
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.
This is so that the symbol can be specified in the increasing_color_map
and decreasing_color_map
, although it shouldn't have filtered to one symbol so I'll fix that.
@@ -69,7 +69,7 @@ def _setup_listeners(self) -> None: | |||
for table, node in self._partitioned_tables.values(): | |||
listen_func = partial(self._on_update, node) | |||
# if a table is not refreshing, it will never update, so no need to listen | |||
if table.is_refreshing: | |||
if table and table.is_refreshing: |
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.
Might want to have this fix as a separate PR just so it's clear this is fixed in the release notes (is there a ticket for it? When do we run into this issue?)
@@ -30,8 +31,10 @@ def indicator( | |||
suffix: str | None = None, | |||
increasing_text: str | None = "▲", | |||
decreasing_text: str | None = "▼", | |||
number_format: str | None = "#,##0.00", |
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.
Autodoc isn't loving this:
ValueError: Missing documentation for deephaven.plot.express.indicator parameters {"##0.00'"}. Verify that parameter names have leading asterisks in the description.
However you're parsing the args, probably just need to check that the comma is at the end, as it seems to pick up the comma in the string right now. Fix that issue in autodoc in a PR before this PR goes through.
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.
Since I'm removing this default this won't block here, but I'll open another ticket because that needs to be solved eventually.
# this should be safe as it shouldn't appear naturally in a d3 format string | ||
# https://github.com/d3/d3-format/tree/v1.4.5#d3-format | ||
# but isn't a perfect solution | ||
FORMAT_PREFIX = "DEEPHAVEN_JAVA_FORMAT" |
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.
I kind of want an =
at the end just to denote the end of the "prefix".
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.
done
I have added some new changes, especially with regards to number format and title. I still need to add unit and e2e tests for these new changes. This PR is blocked on deephaven/web-client-ui#2352 as well. |
These are for deephaven/deephaven-plugins#1088 Currently the layout is only set once, but with indicator it may change, although specifically the title, so I added an event for that. Not sure if it's the best approach.
Adds indicator chart for DH-18091
Note that examples might not work correctly because of deephaven/deephaven-core#6547
Here is some working examples