Skip to content

make Axes._parse_scatter_color_args static #12739

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 24, 2018
Merged

make Axes._parse_scatter_color_args static #12739

merged 1 commit into from
Nov 24, 2018

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Nov 4, 2018 •
edited
Loading

PR Summary

Continuation of #11663. In particular point 2. from #11663 (comment)

This refactors Axes._parse_scatter_color_args to be static by extracting the sole remaining Axes dependency to a color-getter function, which is passed as an argument.

This allows to explicitly test the parser logic without the need to generate a full figure for each test.

The existing test test_scatter_c() is rewritten accordingly. It's currently still limited to the original defaults (4 elements) and does still only do pass/fail testing. But that's to be expanded upon in future PRs.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • Documentation is sphinx and numpydoc compliant

@timhoffm timhoffm force-pushed the axes-parse-scatter-color-static branch from aa975c2 to d6f7ccc Compare November 4, 2018 21:58
jklymak
jklymak reviewed Nov 5, 2018
View reviewed changes
lib/matplotlib/axes/_axes.py
def _parse_scatter_color_args(self, c, edgecolors, kwargs, xshape, yshape):
@staticmethod
def _parse_scatter_color_args(c, edgecolors, kwargs, xshape, yshape,
get_next_color_func):
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this be an optional kwarg to save the mysterious kwarg in the tests?

If the kwarg stays in the tests, can you comment that its a dummy function...

We had someone be pretty firm that the tests should be of user-facing features, and not testing internal methods. I'm not sure I 100% buy that idea, but it is something to consider here. Does testing ax.scatter directly tests that no-one messes up the parsing before or after this fcn?

Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't this be an optional kwarg to save the mysterious kwarg in the tests?

The only usage of this function is to extract the parameter parsing, and in that call the argument is mandatory. I don't want to modify the signature to make the impression that a general use is possible without this parameter. The tests don't use it because we control inputs and thus the code path, but that's rather a consequence of the things we want to test and shouldn't affect the signature.

We had someone be pretty firm that the tests should be of user-facing features, and not testing internal methods. I'm not sure I 100% buy that idea, but it is something to consider here. Does testing ax.scatter directly tests that no-one messes up the parsing before or after this fcn?

Generally I agree that you should only test the public interface. However, this is really expensive here, because you would have to generate a plot for each parameter. Since the color parameters are quite involved, they need a lot of testing. So we're splitting the function into two parts: 1. parse the color arguments to facecolors and edgecolors, 2. draw with these colors.

Part 1 can be tested separately and efficiently. It's still implicitly part of the public API. If something goes wrong in here, the plot will be broken. Part 2 needs testing as well to make sure no-one messes up the parsing before or after part 1. But that can happen with just a few simple tests. Also this should test the actual plotting.

The modified tests were only a subset of part 1 anyway. They did only smoke-test if the parameters were accepted or not.

jklymak
jklymak approved these changes Nov 5, 2018
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

I agree w. the rationale for this to test the private fcn rather than build a plot for each of the many many tests. Its probably also worth the complexity of the new approach....

I still think this could use a couple more comments, because otherwise, if one reads the code w/o the PR description, one wonders why a function is being passed to the function rather than just the color, and I could imagine someone wanting to streamline the akwardness... But I'm not going to block on this.

@timhoffm timhoffm force-pushed the axes-parse-scatter-color-static branch from d6f7ccc to 54ebda9 Compare November 18, 2018 10:52
@timhoffm
Copy link
Member Author

Added a comment why get_next_color_func is a callable rather than a simple color value.

@timhoffm timhoffm added this to the v3.1 milestone Nov 18, 2018
@anntzer
Copy link
Contributor

anntzer commented Nov 22, 2018

Does the performance gain of not creating a new figure actually matter (i.e., is it worth the additional complexity)? Would be nice to have some timings.

@timhoffm
Copy link
Member Author

Figure creation is relatively slow.

def make_fig():
     fig = plt.figure()
     ax = plt.gca()
     ax.scatter([0, 1, 2], [1, 3, 2])
     plt.close(fig)

%timeit make_fig()
12.8 ms ± 549 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

Since the test has a parametrization with 33 cases, this is 400ms. I assume that _parse_scatter_color_args() iteself doesn't take any relevant amount of time, so all of that time goes into the figure creation and teardown.

400ms may not be much regarded in isolation. However, the parameter space is not tested completely. We may add some more cases. Also, If we can make it a habit of not generating figures in tests if we don't need them, I think we can save relevants amounts of time in testing.

@anntzer
Copy link
Contributor

anntzer commented Nov 24, 2018

Honestly I'm not a fan of the API this introduces, which is basically complicated by the fact that there is no API to peek at the next color of the color cycle without advancing the iterator (that would actually be a worthwhile addition). Even with 10x more test cases, we're talking about speeding up the test suite by 4s...
I'm not (even close to) going to block this PR of course, but I'm unconvinced by it.

@timhoffm
Copy link
Member Author

Peaking into the colorcycle doesn't help very much here. Even if you could get the next color, _parse_scatter_color_args() would have to return if it really used it so that you can adjust the colorcycle.

I assume that there are many more tests that actually would not need a figure. If we establish a pattern of not creating figures if we don't really need them and apply it throughout the tests, I expect that we can make the test suite measureably faster.

This looks a bit more complicated on the first view, however it's actually more clear. Making the function static isolates it's effect from the Axes instance. There cannot be any change of hidden state anymore.

@anntzer
Copy link
Contributor

anntzer commented Nov 24, 2018

Even if you could get the next color, _parse_scatter_color_args() would have to return if it really used it so that you can adjust the colorcycle.

I missed this very good point.

@anntzer
Copy link
Contributor

anntzer commented Nov 24, 2018

Let's see how much speedup we can gain :)

@anntzer anntzer merged commit 220b1ef into matplotlib:master Nov 24, 2018
@timhoffm timhoffm deleted the axes-parse-scatter-color-static branch November 24, 2018 16:19
@rcomer rcomer mentioned this pull request Nov 14, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anntzer anntzer anntzer approved these changes

@jklymak jklymak jklymak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@timhoffm @anntzer @jklymak