Consistent grid sizes in streamplot. #2700
Conversation
| @@ -28,7 +28,8 @@ def test_linewidth(): | |||
| X, Y, U, V = velocity_field() | |||
| speed = np.sqrt(U*U + V*V) | |||
| lw = 5*speed/speed.max() | |||
| plt.streamplot(X, Y, U, V, density=[0.5, 1], color='k', linewidth=lw) | |||
| df = 25. / 30. | |||
There was a problem hiding this comment.
I'd add a note here. Something like:
df = 25. / 30. # Compatibility factor for old test image
|
Minor comments above. Otherwise 👍 |
|
Thanks for the quick review @tonysyu, I've addressed your comments. I overwrote the previous commit since the changes were few and minor, hope that is OK. |
|
Thanks. This looks ready to merge, but I don't actually have the power of the green button so it'll have to wait for a core-dev. |
|
I don't want to be too fussy when something might be ready to go, but wouldn't it be better to update the test images than to put "compatibility factors" in the tests? |
|
I don't know if it would be better actually. It is just as simple to slightly modify the tests and avoid having to commit new test images. They could be described differently in the comments of course, I'm not sure I like the term "compatibility factor". They could be described as factor necessary to get a 25x25 grid etc. |
|
The naming is odd, but I think changing the test this way is a feature, not a bug, as it will catch more issues than testing A bigger problem is that the grid dimension is hard coded, it should really be a kwarg. @ajdawson would you be interested/willing to make that change as well (in a different PR against master)? |
|
I don't see any need for another kwarg; density is already controlling this underlying parameter (assuming you are referring to "30", @tacaswell). One might argue that instead of a "density" kwarg, there should have been a "grid" kwarg, defaulting to (30, 30), perhaps with the dreaded rcParam to set the default. |
|
Sorry, Eric is right, I wrote that last comment to early and did not fully grasp what density was doing. It would be better to replace it with 'gridsize' or some such which directly sets This looks fine to me +1. |
Consistent grid sizes in streamplot.
This PR addresses #2653. The grid size used by
streamplot()fordensity=1was 30x30 (not 25x25 as stated in the docstring) but fordensity=(1, 1)it was 25x25. This PR changes the base grid size for both density specification types to 30x30, since this is the grid size currently used for the default value of thedensityargument.I modified two tests that use a vector density specification so that the resulting test plots are the same. I saw no need for new test images taking up space in the repository.