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

Fix plot-series #533

Merged
merged 1 commit into from Dec 13, 2020
Merged

Fix plot-series #533

merged 1 commit into from Dec 13, 2020

Conversation

gracewgao
Copy link
Contributor

Reference Issues/PRs

Fixes #519

What does this implement/fix? Explain your changes.

As the issue points out, the x-axis labels don't match with the data points plotted. It turns out that mathplotlib recommends a different solution for dynamically assigning tick labels from a list. I tweaked this code a bit to accommodate multiple series, which gives this result:

y = load_airline()
y = y[:48]
fh=np.arange(1, 13)

y_train, y_test = temporal_train_test_split(y, test_size=len(fh))
plot_series(y_train, y_test, labels=["y_train", "y_test"]);
print(y.shape, y_train.shape[0], y_test.shape[0])
print(y.index)

(48,) 36 12
PeriodIndex(['1949-01', '1949-02', '1949-03', '1949-04', '1949-05', '1949-06',
'1949-07', '1949-08', '1949-09', '1949-10', '1949-11', '1949-12',
'1950-01', '1950-02', '1950-03', '1950-04', '1950-05', '1950-06',
'1950-07', '1950-08', '1950-09', '1950-10', '1950-11', '1950-12',
'1951-01', '1951-02', '1951-03', '1951-04', '1951-05', '1951-06',
'1951-07', '1951-08', '1951-09', '1951-10', '1951-11', '1951-12',
'1952-01', '1952-02', '1952-03', '1952-04', '1952-05', '1952-06',
'1952-07', '1952-08', '1952-09', '1952-10', '1952-11', '1952-12'],
dtype='period[M]', name='Period', freq='M')

image

Does your contribution introduce a new dependency? If yes, which one?

No, I just imported some new functions from mathplotlib.ticker and mathplotlib.cbook.

PR checklist

For all contributions
  • I've added myself to the list of contributors.
  • Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
  • I've added unit tests and made sure they pass locally.
For new estimators
  • I've added the estimator to the online documentation.
  • I've updated the existing example notebooks or provided a new one to showcase how my estimator works.

Any other comments?

I'm not too sure what parts of the PR checklist are relevant - feel free to let me know if there are any changes / additional actions that I should take!

@mloning
Copy link
Contributor

mloning commented Dec 12, 2020

@gracewgao this looks like a nice fix!

Would you like to add a unit test too? If yes, we could merge this PR first or you could add it to this one, whatever you prefer.

If you have time, could you also check if it works as expected with integer indices? For example, here the index is wrong:

from sktime.forecasting.all import *
from sktime.utils._testing.forecasting import _make_series

y = _make_series(index_type="int")
print(y.index)
>>> Int64Index([ 3,  4,  5,  6,  7,  8,  9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
            20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36,
            37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52],
           dtype='int64')
plot_series(y)

image

@gracewgao
Copy link
Contributor Author

I just checked, and it works with integer indices too!

image

And sure, I can add a unit test. Maybe we can merge this PR first and then I'll make another PR with the unit test?

Copy link
Contributor

@mloning mloning left a comment

Choose a reason for hiding this comment

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

Alright - yes so this PR looks great! I'll leave it open for a bit longer in case someone else wants to comment but it's ready to be merged from my side!

For the unit test, the main thing to check is the x-axis I think for different input combinations.

@mloning mloning merged commit 28646ab into sktime:master Dec 13, 2020
@mloning
Copy link
Contributor

mloning commented Dec 13, 2020

Thanks again @gracewgao - now merged!

@wolfiex
Copy link

wolfiex commented Jan 13, 2021

This issue seems to still persist in version 0.5.2. Can I ask how I would import the changes?

I am plotting the output of temporal_train_test_split which is correct in everything but the plot x-axis.

@mloning
Copy link
Contributor

mloning commented Jan 14, 2021

@wolfiex thanks for reaching out! Can you post a reproducible example on #519 to better understand what's going on? I can reopen the issue then

@wolfiex
Copy link

wolfiex commented Jan 14, 2021

@mloning a screenshot is attached.

import sktime
sktime.__version__

# returns '0.5.2'

Screen Shot 2021-01-14 at 18 22 37

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.

[BUG] plot_series inconsistent date/time index
3 participants