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
allow slice and fancy indexing to only show some markers #2662
allow slice and fancy indexing to only show some markers #2662
Conversation
every=slice(i, j + 1, j - i) otherwise every j-th point starting | ||
at point i will have a marker. | ||
|
||
ACCEPTS: None | integer | (startind, stride) | slice object | list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as you are touching the doc-string can you re-write it in the numpy-doc style?
All of the test failures are PEP8 violations (specifically trailing white space). Your editor should have an option to truncate trailing white space on save. It is also worth your time to figure out how to get in-line pep8 testing in your editor. For better or worse, one of our tests is pep8 compliance, taking care of it as you code saves time. |
# as a work-around use markevery = slice(i,j+1,j-i) | ||
# to show only point numbers i and j. | ||
startind, stride = markevery | ||
inds = slice(startind, None, stride) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the behaviour where if you just so happen to have 2 indices the behaviour changes.
I'd be tempted to raise a warning here stating that the behaviour has changed and in the warning give an example of how to use the new behaviour, but no matter what, an iterable is considered to be a fancy index.
Obviously this is a backwards incompatible change, so we will need an entry in the docs/api/api_changes.rst
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be left as it is (len 2 -> old method) but with a warning for a release (1.4) before being change over entirely to the new behavior.
Changing how a length 2 iterable is interpreted will break too much existing code.
It would add more complexity, but it might be worth adding an rcparam for newstyle vs oldstyle to smooth over the transition.
Here's a newbie question: I've added a bit more functionality to my code above to allow markers at a roughly fixed display coordinate distance along the plot; should I push my changes to this pull request or do a completely new pull request? |
Depends. I think this is a good feature to have and should be merged (modulo you cleaning up pep8 and adressing @pelson 's concerns about length 2 iterabels being special). Adding more (related) features to this PR could make it take longer to get sorted and merged, which argues for making a new PR. On the other hand, if the additional code depends on this PR, then it can't be merged first and you will probably end up rebasing it multiple times due to changes from fixing this PR. The new PR would basically be on hold until this one is sorted out, so might as well add it into this one. On the third hand, if the extra features are independent of this PR, then put them in their own PR (but you might still end up having to rebase repeatedly). Evenly spaced markers in figure space does sound useful. |
I've done two things in my lastest commit 54906e3.
Here's some example code and plots:
|
if isinstance(markevery, float): | ||
markevery = (0.0, markevery) | ||
if isinstance(markevery, tuple): | ||
start, step = markevery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should raise a value error if len(markevery) != 2
instead of users getting the cryptic error about too many/few values to unpack.
This is good stuff! I do not share your faith in users reading the documentation (mainly due to answering SO questions, a good fraction of which you can answer by copy/pasting the documentation). I left some comments in the code, if we are going to have such complex type handling then it needs to be bullet proof (in part because users don't actually read the documentation). The test failure for pep8 need to be cleaned up before this can be merged. Can you add some or all of those very nice examples as tests? |
inds = np.unique(inds) | ||
elif isinstance(markevery, int): | ||
start, step = 0, markevery | ||
inds = slice(start, None, step) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be an elif iterable
and then have a final else
which raises a ValueError
I'm out of my comfort zone with the testing I added in commit 0838b94 and b55fb1c. I didn't touch the existing markevery tests in test_axes.py, but added some more to test the new functionality. I'm not really sure what happened with my branch merging but I think I got it sorted. It's still possible to sneak through the type checking and ValueError checking. e.g. markevery='hello' will sneak through because a string is iterable. How many cases do we have to checkfor? |
inds = markevery | ||
elif iterable(markevery): | ||
#fancy indexing | ||
inds = markevery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a test here that all elements of markevery
are integers. This addresses markevery = 'hello'
and markevery = [np.pi, .5]
.
I am not sure the best way to do this test.
At a quick glance the test images look good to me and they are passing on travis. I don't understand what is going on with that last commit on this branch. As it seems to be a no-op, I would just point your local branch at the commit before and use There are still pep8 issues:
|
Hopefully I've cleared up the pep8 errors. To check for a valid iterable, because there are a number of valid ways to specify a valid numpy fancy index (list of int, array of int, list of bool etc. ) I put in a try except block. This ultimately results in some duplication in that for a valid iterable |
functionality as `every`=0.1 is exhibited but the first marker will | ||
be 0.5 multiplied by the display-cordinate-diagonal-distance along | ||
the line. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do this as a bullet list?
This is great functionality, but the behavior change based on the type of I've tagged it as 1.4 so that it gets looked at by the core devs soonish. |
I reworked the parameters section of set_markevery into a more readable bullet list. I had some trouble using backticks for the |
|
||
Notes | ||
----- | ||
Using `markevery` will only show markers at actual data points. When |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be every
not markevery
? In either case, it should be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure which to use. every
is the argument to the method set_markevery
whereas markevery
is a keyword argument of plt.plot. I'll try and delineate between every
the method argument, and the markevery property
.
@rtrwalker Will you be able to address the comments in the near future? |
note to self: once this is merged someone should add an answer to https://stackoverflow.com/questions/8409095/matplotlib-set-markers-for-individual-points-on-a-line advertising this feature. |
I've been out of action for a bit. I will address the comments soon. |
😄 - well here's one from me - I'd like to remove the logic from the draw method to keep it readable. @tacaswell - I give you the casting vote. |
I just did it my self. I thought it should be done, but felt bad telling @rtrwalker to do it. There is a PR against this branch. |
I'm not sure why the last travis build (commit 33ca595) failed. It passes for python 2.6, 2.7 and 3.3; it fails for python 3.2. Looking at the log, failure occurs for the multipe cases of |
That is in un related to this pr
|
@rtrwalker Can you merge my PR against your branch so we can move this forward? |
erm, sorry for the noise, I am confused by the new github page layout and now see it is already done. What this is missing still is entries in CHANGELOG, whats_new.rst and api_changes.rst. |
@rtrwalker Can you add the documentation details so we can get this merged? |
@rtrwalker Can you rebase this again? |
@tacaswell This has been rebased, I think. I would love to see this added. |
It also needs the docs which is going to end up being my problem. It claims to still not merge cleanly... |
@rtrwalker Can you rebase this (yet) again? |
…nto tuple and list/array
patched up conflict by hand in 9d8561c |
The
markevery
keword argument can be passed to plt.plot in order to plot a marker every N points. I've expanded the functionality to allow using slices and fancy indexing to specify specific points to plot the markers with. This avoids having to plot two series (the line, and the markers) when putting a marker on a custom subset of points. Goes someway to addressing:#1981
http://stackoverflow.com/questions/5318639/matplotlib-ugly-plots-when-using-markers
http://stackoverflow.com/questions/17406758/plotting-a-curve-with-equidistant-arc-length-markers
http://stackoverflow.com/questions/2040306/plot-with-fewer-markers-than-data-points-or-a-better-way-to-plot-cdfs-matplo
http://stackoverflow.com/questions/8409095/matplotlib-set-markers-for-individual-points-on-a-line
for example: