-
-
Notifications
You must be signed in to change notification settings - Fork 420
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
pass pair plot scatter_kwargs to plt.scatter(), not plt.plot() #1889
Comments
Hey can you please elaborate this problem and what exactly is that you are trying to achieve and can you post any screenshots of the problem. |
iiuc, the main goal is being able to use the That would mean updating the docs at Line 100 in 9d74000
scatter so bonus points for fixing a bug we didn't know we had 😅 and then it should be passed to ax.scatter instead of ax.plot in https://github.com/arviz-devs/arviz/blob/main/arviz/plots/backends/matplotlib/pairplot.py#L177 and https://github.com/arviz-devs/arviz/blob/main/arviz/plots/backends/matplotlib/pairplot.py#L177
Side note, if you are not comfortable with git merging and rebasing, it's probably a good idea to wait for #1985 to be merged before starting to work on this. |
Hey, so I looked a bit into this and
And the reason why dealisaing is assuming that this is passed down to scatter is because |
The scatter_kwargs with a
The dealiasing should not be changed. It will be correct once the issue is fixed, but is currently incorrect because it uses "scatter" as the second argument instead of "plot", but those arguments are incorrectly passed to an |
It worked without my changes and isn't dealiasing using scatter keyword only when |
Hi @tattvams, It doesn't currently work. Let me illustrate. The following code generates random import matplotlib.pyplot as plt
import numpy as np
rng = np.random.default_rng(1234)
x, y = rng.uniform(0, 10, size=(2, 100))
z = x + y
plt.scatter(x, y, c=z)
plt.show() If you change the
Your example above with |
Oh ok, I guess then there should be a problem with matplotlib backend because in "c" keyword is used and then passed to scatter which returns PathCollection |
It is not an issue with matplotlib. In scatter we want to plot a collection of dots, not necessarly ordered, where the dots themselves might contain more information than merely their position. This information can be encoded in the size or color of the dots which are therefore independent properties between dots. They are accessed with In plot we want to plot an ordered sequence of values, so it defaults to a line plot. It is customizable enough for us to use markers and no lines and generate plots that look like scatter plots, but that is not the goal it's more of a side effect. plot should be used if we care about the position of the dots and the relation to the previous and next dots/values. It has a Now the caveats:
I hope that provides more context on why the dealiasing was wrong and why using plot instead of scatter is an issue here. Both issues on ArviZ side |
hey, by backend I didn't mean matplotlib what i meant was maybe there was a reason that c didn't accept any values other than rgb because it may have failed to add cmap functionality which i understand is not the case and that it went to plt.plot() instead of plt.scatter() and can you send me source code where .plot() function is defined because I am unable to find it. Thanks! |
Do you mean this https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.plot.html? |
I think originally we went with plot interface was due to speed (scatter was slower for a huge number of points). I don't know what is the current standing, but I think going with .scatter solution is fine. Here is source for .scatter and here source for .plot |
Maybe setting The main reason I didn't immediately send a patch fixing this in arviz is because I wasn't sure if there were backwards-compatibilitiy concerns over such a change. Figure size/speed issues would also be a bad behaviour change. |
We could probably add that as an default. Tbh I think scatter is fast enough these days. Speed is always an issue when handling large datasets, but in most cases the datasets are not that large. |
closed by #2069 |
I want to do a pair plot with a colour map, where the colour of the points indicates the posterior probability. I expected to just be able to pass
scatter_kwargs={"c": probs}
to arviz.plot_pair(), but it seems the matplotlib backend uses plt.plot() instead of plt.scatter(), and plt.plot() doesn't seem to support this (I think?). Also, it's kinda confusing that this is namedscatter_kwargs
, when it doesn't use the scatter function.The text was updated successfully, but these errors were encountered: