-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conversation
@cpranav93 Could you have a look and let me know what you think? |
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.
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! 🎆
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.
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.
Oh, we are using pylint 😬 I will address this and then merge. Thanks!
|
@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. |
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. |
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:This is what it looks like:
I tried to keep the interface somewhat basic, because I expect this will need some finetuning after #476 and #490
Closes #457