Skip to content

remove _ensure_plottable #5763

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

dschwoerer
Copy link
Contributor

The plotting backend does more reliable checking and thus removing avoids
false negatives, which are causing easily avoidable plot failures

  • Closes Plotting of labelled data fails #5762
  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

The plotting backend does more reliable checking and thus removing avoids
false negatives, which are causing easily avoidable plot failures
@pep8speaks
Copy link

pep8speaks commented Sep 3, 2021

Hello @dschwoerer! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 4:1: F401 'datetime.datetime' imported but unused

Comment last updated at 2021-10-24 10:33:03 UTC

@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2021

Unit Test Results

         6 files           6 suites   53m 30s ⏱️
16 209 tests 14 462 ✔️ 1 736 💤 11
90 450 runs  82 204 ✔️ 8 180 💤 66

For more details on these failures, see this check.

Results for commit 8ecbb39.

♻️ This comment has been updated with latest results.

@dcherian
Copy link
Contributor

dcherian commented Sep 3, 2021

Thanks @dschwoerer This makes sense to me.

A lot of failed tests are checking for the raised error. We will need to update these to make sure the plotting worked.

@dschwoerer
Copy link
Contributor Author

I think there are several options:

  1. remove the offending tests (simple)
  2. reintroduce _ensure_plottable but only check for multiindex. This has still the issue that it might at some point be possible, and the tests need to be changed and _ensure_plottable needs to be changed
  3. Change the test to check for any error. This basically just means we are checking it isn't working, which I see only limited value in.

Which of these options would you prefer?

@dcherian
Copy link
Contributor

dcherian commented Sep 3, 2021

The error for multiindex plotting is not very informative "setting array element with sequence" so I would still keep _ensure_plottable to raise a nice error for that.

for the tests that now work, we should check that the output is reasonable, so I wouldn't delete them

@Illviljan
Copy link
Contributor

What would you expect the plot to look like if you input a complex number or a multiindex along the x-axis?
I think I would expect it to be shown like a flattened array. And you can kind of get away with that if you cast the arrays to string before hand.

x = 1j * np.arange(0, 5)
y = np.arange(5, 10)
plt.plot(x, y)
ComplexWarning: Casting complex values to real discards the imaginary part
  return np.asarray(x, float)

image

x = 1j * np.arange(0, 5)
y = np.arange(5, 10)
plt.plot(np.vectorize(str)(x), y)

image

x = 1j + np.arange(0, 5)
y = np.arange(5, 10)
plt.plot(np.vectorize(str)(x), y)

image

import pandas as pd
arrays = [[1, 1, 2, 2], ['red', 'blue', 'red', 'blue']]
a = pd.MultiIndex.from_arrays(arrays, names=('number', 'color')).to_numpy()
b = np.arange(4, 8)
plt.plot(np.vectorize(str)(a), b)

image

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.

Plotting of labelled data fails
4 participants