Skip to content
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

Add visualization for timeseries #491

Merged
merged 6 commits into from
Mar 1, 2023
Merged

Add visualization for timeseries #491

merged 6 commits into from
Mar 1, 2023

Conversation

stefsmeets
Copy link
Contributor

This PR implements a visualization for timeseries visualization. I tried to implement what seems to be the most common way of making these plots.

Currently it takes x, y and a bunch of segments that look like this:

from dianna.visualization import plot_timeseries

segments = [
    {'index': 0, 'start': 0, 'stop': 2, 'weight':-0.6},
    {'index': 1, 'start': 2, 'stop': 4, 'weight':-0.3},
    {'index': 2, 'start': 4, 'stop': 6, 'weight': 0.0},
    {'index': 3, 'start': 6, 'stop': 8, 'weight': 0.4},
    {'index': 4, 'start': 8, 'stop': 10, 'weight': 0.7},
]

x = np.linspace(0, 10, 20)
y = np.sin(x)

plot_timeseries(x=x, y=y, segments=segments, cmap='bwr', show_plot=True)

This is what it looks like:

Screenshot 2023-02-28 162944

I tried to keep the interface somewhat basic, because I expect this will need some finetuning after #476 and #490

Closes #457

@stefsmeets stefsmeets marked this pull request as ready for review February 28, 2023 15:30
@stefsmeets
Copy link
Contributor Author

@cpranav93 Could you have a look and let me know what you think?

Copy link
Contributor

@cwmeijer cwmeijer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great and I also like the input format. I think it is perfect.
I don't suppose this works on multivariate time series right? That is totally fine, but I think the function title (or at least the doc string) should mention this.
Also, please fix the linting before merging. :-)
Great addition! 🎆

dianna/visualization/timeseries.py Outdated Show resolved Hide resolved
Copy link
Contributor

@cpranav93 cpranav93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! As we expand the time-series functionality in Dianna we can add more methods for visualisation if necessary. Prospector seems to be failing for the build - maybe worth a look before merging.

@stefsmeets
Copy link
Contributor Author

Oh, we are using pylint 😬 I will address this and then merge. Thanks!

dianna/visualization/timeseries.py
  Line: 7
    pylint: too-many-arguments / Too many arguments (9/5)
    pylint: too-many-locals / Too many local variables (16/15)

tests/test_visualization.py
  Line: 5
    pylint: missing-function-docstring / Missing function or method docstring
  Line: 18
    pylint: trailing-newlines / Trailing newlines

@stefsmeets
Copy link
Contributor Author

@cwmeijer Multiviarate timeseries can be easily added later. I would suggest to add that once it is more clear what data needs to be plotted.

@cwmeijer
Copy link
Contributor

cwmeijer commented Mar 1, 2023

I absolutely agree with adding multivariate functionality later. Please still update the doc string because who knows how many releases we will do before actually adding multivariate support.

@stefsmeets stefsmeets merged commit 5038beb into main Mar 1, 2023
@stefsmeets stefsmeets deleted the plot_timeseries branch March 1, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define visualization for time series data domain
3 participants