569 multivariate timeseries visualization #591
Conversation
Minor style improvements in plot_timeseries
…m:dianna-ai/dianna into 569-multivariate-timeseries-visualization
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
…m:dianna-ai/dianna into 569-multivariate-timeseries-visualization
|
@loostrum do you have a chance to review this? |
There was a problem hiding this comment.
The graphs look great! But I'm not entirely happy with this PR.
-
The docstring for
plot_timeseriesdoes not entirely cover the functionality. Could you update it to make it more clear how the function works with multivariate data? -
The interface is not the same as before for monovariate data. To me it was not directly obvious what changes I needed to make to get it to work. I had to fiddle with x and y, and adjust the segments to get it to work. Better documentation and checks to make it clear what is wrong would help.
That said, I'm thinking that it may be worth to consider splitting plot_timeseries into plot_timeseries and plot_timeseries_multi to keep the API clear. To me, this function is not intuitive at the moment. Part of the reason is also having to define segments each time to call it, this could be integrated into the explain_timeseries function. Not to be addressed in this PR, but definitely something to follow up on.
- Can you also have another look at the notebooks? The notebooks break whenever the
plot_timeseriesfunction is called on my end:
ValueError: x and y must have same first dimension, but have shapes (28,) and (1,)
I think either the segments are not correctly defined or the shape of the input data is not right.
- I also left a couple of nitpicks that may improve the code clarity.
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
…m:dianna-ai/dianna into 569-multivariate-timeseries-visualization
| @@ -6,7 +6,7 @@ | |||
|
|
|||
| def test_common_RISE_image_pipeline(): # noqa: N802 ignore case | |||
| """No errors thrown while creating a relevance map and visualizing it.""" | |||
| input_image = np.random.random((5, 5, 3)) | |||
| input_image = np.random.random((224, 224, 3)) | |||
There was a problem hiding this comment.
This would be a regression from #606 which we merged yesterday :-)
| input_image = np.random.random((224, 224, 3)) | |
| input_image = np.random.random((5, 5, 3)) |
Adds multivariate support to the timeseries visualization: