feat: allow LinePlot to be drawn in steps#99
Open
larshei wants to merge 6 commits intomindok:masterfrom
Open
Conversation
Owner
|
Thanks @larshei - it looks great! It is a breaking change to the API so what I might do is add back the Edit: Just saw the last commit 🙄 |
mindok
reviewed
Aug 14, 2024
lib/chart/lineplot.ex
Outdated
| # keep backwards compatibility with old `:smoothed` option | ||
| style = get_option(plot, :plot_style) |> IO.inspect(label: "style") | ||
| smoothed = get_option(plot, :smoothed) |> IO.inspect(label: "smoothed") | ||
| plot_style = if smoothed != nil, do: smoothed, else: style |
Owner
There was a problem hiding this comment.
Should this be:
plot_style = if smoothed != nil, do: :smooth, else: style
(i.e. :smoothed atom set)
Owner
There was a problem hiding this comment.
Ah - I see what you've done - the true/false is handled in the path logic. I think it's better to definitively set the plot_style here for a couple of reasons:
- We isolate the deprecation logic to a single spot
- The paths code may be used from elsewhere in the future and the options (true, false, :smooth, :direct, :step) are overlapping and unclear
- There are fewer places to go to remove the deprecated code
so something like:
plot_style = case smooth do
true -> if smoothed, do: :smooth, else: :direct
_ -> style
end
Author
There was a problem hiding this comment.
Adjusted to
plot_style =
case smoothed do
true -> :smooth
false -> :direct
_ -> style
end
mindok
reviewed
Aug 14, 2024
Owner
|
Thanks @larshei - I've set aside some time to take a look on Monday. Apologies for the slow responses. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Many timeseries depict a "state" of some sort that was measured at specific points in time.
Simply drawing a line between the points of measurement often feels weird to me. I rather think "the value has been y from t1 to t2, then at t2 we got a new value".
So I added a style for line graphs that creates a LinePlot with steps:
As we no longer have two options (smooth yes/no) but three options,
the interface needed to be changed:
The option is now called
plot_type, with the options:direct:smooth:stepIf
plot_typeis not provided, we fall back to thesmoothedoption.