Skip to content
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

Fixed issues with errorbar limits #2452

Merged
merged 8 commits into from Apr 4, 2014
Merged

Conversation

larrybradley
Copy link
Contributor

This fixes two issues with errorbar limits. An upper limit means the exact value of a data point is unknown, but it is known not to be higher than a particular value. This is denoted by an downward arrow whose base is at the value of the limit. Similarly lower limits are upward arrows. This patch corrects the arrow directions (now flipped) and makes the arrow base at the data point (the arrow is no longer straddling the data limit). I've also included a new example file.

I think it would also be nice to add keywords to control the arrow size, face color, etc., but that is not implemented here.

@mdboom
Copy link
Member

mdboom commented Sep 30, 2013

@efiring, @WeatherGod: git blame tells me you guys are reasonably well versed with the errorbar code. Any comments?

plot_kw['markeredgewidth'] = capthick
# For backwards-compat, allow explicit setting of
# 'mew' or 'markeredgewidth' to over-ride capthick.
if 'markeredgewidth' in kwargs:
Copy link
Member

Choose a reason for hiding this comment

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

While you are at it, you might as well condense this sequence down to:

for key in ('markeredgewidth', 'mew', 'transform', 'alpha', 'zorder'):
    if key in kwargs:
        plot_kw[key] = kwargs[key]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good idea.

@WeatherGod
Copy link
Member

due to a bunch of things that have fallen onto my plate the past few days,
I am going to be swamped. My initial reaction from a cursory glance is that
I think I need to see some sort of authoritative source indicating which
way the arrows should go. Also, a test is needed, of course.

@tacaswell
Copy link
Member

@larrybradley Could you possibly add a test to this?

@tacaswell
Copy link
Member

@larrybradley This needs a re-base. Any progress on the reference?

@larrybradley
Copy link
Contributor Author

@tacaswell Is common sense not enough? :) For example, an upper limit is a maximum possible value (e.g. http://dictionary.reference.com/browse/upper+limit), meaning the possible data values are below the upper limit value. This is naturally plotted as a downward-facing arrow starting at the upper limit value.

I tracked down when the upper/lower limits code was first added to matplotlib in Sep 2007, and it appears to have simply been implemented wrong.
From http://www.mail-archive.com/matplotlib-devel@lists.sourceforge.net/msg01611.html
"In such a case one would use an arrow pointing downwards to indicate that this measurement only gives an upper limit for the mass."

This is a correct statement, but unfortunately it was implemented such that the arrow directions were reversed.

Does commonly-used ~22-year old IDL astronomy software count as a reference?
http://idlastro.gsfc.nasa.gov/ftp/pro/plot/plotsym.pro

@tacaswell
Copy link
Member

I do not work with these types of graphs and I have found that 'common sense' is very subjective (see the difference of meaning in red/green in stock quotes between the west and the east).

I would like @WeatherGod to OK this before merging.

Travis is still failing, but is not letting me see the logs....

@@ -0,0 +1,53 @@
"""
Demo of the errorbar function, includig upper and lower limits
Copy link
Member

Choose a reason for hiding this comment

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

spelling error here

@WeatherGod
Copy link
Member

I am a bit confused. I thought that this patch was going to make the arrows point inward. The example image shows them pointing outward, which I think is current behavior? Just wanted to make sure that that was what was intended.

@larrybradley
Copy link
Contributor Author

Thanks, @WeatherGod! I've fixed the typo and added the comments. The example image shows examples of both lower and upper limits in both x and y.

@larrybradley
Copy link
Contributor Author

@tacaswell Travis failed because of an extra space (pep8 check). It's fixed now.

@WeatherGod
Copy link
Member

@larrybradley I realize what the example image is, I am just saying that the arrows in the example do not match what is described in the description of this PR:

An upper limit means the exact value of a data point is unknown, but it is known not to be higher than a particular value. This is denoted by an downward arrow whose base is at the value of the limit. Similarly lower limits are upward arrows. This patch corrects the arrow directions (now flipped)

From this description, I would have expected the arrows to now point inward, rather than outward, which is what I see in the example PNG image. Am I misinterpreting something?

@larrybradley
Copy link
Contributor Author

errorbar_limits

from examples/statistics/errorbar_limits.py:

# standard error bars
plt.errorbar(x, y, xerr=xerr, yerr=yerr, ls=ls, color='blue')

# including upper limits
uplims = np.zeros(x.shape)
uplims[[1, 5, 9]] = True
plt.errorbar(x, y+0.5, xerr=xerr, yerr=yerr, uplims=uplims, ls=ls,
             color='green')

# including lower limits
lolims = np.zeros(x.shape)
lolims[[2, 4, 8]] = True
plt.errorbar(x, y+1.0, xerr=xerr, yerr=yerr, lolims=lolims, ls=ls,
             color='red')

It looks correct to me. The green arrows are upper limits (uplims), and hence point down. The red arrows are lower limits (lolims), and hence point up. The magenta and cyan points are mixtures of upper/lower limits. @WeatherGod Where does it seem reversed to you?

@tacaswell
Copy link
Member

@WeatherGod I am leaving this PR to you, please merge when you are happy.

@WeatherGod
Copy link
Member

let's dispose of the "up" and "down" words here, as it is easy to get
confused with upper and lower. I might be completely obtuse here, but my
interpretation of the whole problem is that the arrows (denoted by the
triangle markers) were pointing the "wrong" way (as well as being placed
incorrectly). In the picture above, they are pointing "outward". From my
interpretation of the PR description, you want them pointing "inward".

On Tue, Feb 25, 2014 at 2:39 PM, Larry Bradley notifications@github.comwrote:

[image: errorbar_limits]https://f.cloud.github.com/assets/4992897/2261860/d35a9bc8-9e53-11e3-8312-df1ae678938e.png

from examples/statistics/errorbar_limits.py:

standard error bars

plt.errorbar(x, y, xerr=xerr, yerr=yerr, ls=ls, color='blue')

including upper limits

uplims = np.zeros(x.shape)
uplims[[1, 5, 9]] = True
plt.errorbar(x, y+0.5, xerr=xerr, yerr=yerr, uplims=uplims, ls=ls,
color='green')

including lower limits

lolims = np.zeros(x.shape)
lolims[[2, 4, 8]] = True
plt.errorbar(x, y+1.0, xerr=xerr, yerr=yerr, lolims=lolims, ls=ls,
color='red')

It looks correct to me. The green arrows are upper limits (uplims), and
hence point down. The red arrows are lower limits (lolims), and hence
point up. The magenta and cyan points are mixtures of upper/lower limits.
@WeatherGod https://github.com/WeatherGod Where does it seem reversed
to you?

Reply to this email directly or view it on GitHubhttps://github.com//pull/2452#issuecomment-36048807
.

@mdboom
Copy link
Member

mdboom commented Feb 25, 2014

I think the arrows are supposed to remain pointing "outward" -- it's merely that they were pointing in the wrong direction. i.e. an upper limit should have the arrow pointing down.

I think this is definitely a bugfix, but it's enough of a change in behavior, that it definitely deserves an entry in whats_new and maybe api_changes as well.

@WeatherGod
Copy link
Member

"i.e. an upper limit should have the arrow pointing down"

do you mean outward or inward? The up and down notation is useless when
discussing the lower and upper limits for the x values, and I am just
getting utterly confused here.

I am perfectly willing to accept that my interpretation of this entire
discussion is completely and utterly wrong. I have been wrong on things
before. But given how much confusion there is, I would suggest making sure
that any description of this in 'whats_new.rst' and whereelse is
unambiguous.

@larrybradley
Copy link
Contributor Author

I'm sorry for any confusion. The arrow should always point outward, i.e. away from the data value. I think perhaps you are overthinking this. Conceptually, limits are very simple. In the plot below, I show the current matplotlib implementation vs. this PR. Hopefully this helps, because I don't know how to make it any simpler.

uplim

@larrybradley
Copy link
Contributor Author

Please don't merge this yet, as there is a pre-existing bug in errorbar (e.g. current matplotlib also has it) for reversed axes (xlim() or ylim() where the first element is larger than the second. I will submit an updated PR to fix this. Thanks to @mdboom for suggesting that I test for axis reversal!

@WeatherGod
Copy link
Member

Ok, now I see what you mean. Before this example, I thought "arrow"
referred just to the marker, as I consider the line part to just be the
error bars and not part of any arrows.

This is definitely a major change in behavior, though. I am hesistent to
make such a change without some sort of way to switch between them.

@efiring
Copy link
Member

efiring commented Feb 25, 2014

Based on the comparison and explanation provided by @larrybradley, I think the current mpl behavior makes no sense at all, so I don't think there should be a way to switch back to it. All this needs is documentation, as @mdboom noted above

@WeatherGod
Copy link
Member

If I understand all this correctly, wouldn't similar results have been
possible with a simple switch from using ylolim to yhilim to reverse the
direction of the arrow? If so, then I wonder how many people saw an odd
behavior, figured that they misinterpreted the docs, and just switched it?

@efiring
Copy link
Member

efiring commented Feb 26, 2014

@WeatherGod, Yes, people could have switched it. So what? Maybe no one actually used it at all... Who knows?

@tacaswell
Copy link
Member

@WeatherGod and @efiring What do you two want to do about this PR?

@efiring
Copy link
Member

efiring commented Mar 18, 2014

@larrybradley I think this should be merged ASAP, but this requires your PR to fix the pre-existing bug that you mention above, and it also needs entries in whats_new and api_changes, as @mdboom noted above. Will you be able to attend to these things?

@larrybradley
Copy link
Contributor Author

@efiring Sure, I'll try to finish the PR this week. I've been busy with other things.

@larrybradley
Copy link
Contributor Author

This should be ready to merge now.

@tacaswell
Copy link
Member

@larrybradley Can you rebase this? I suspect there are conflicts in the documentation files.

@larrybradley
Copy link
Contributor Author

@tacaswell OK, rebased now.

@tacaswell
Copy link
Member

@efiring Can you do the honors on this one?

@efiring
Copy link
Member

efiring commented Mar 26, 2014

@larrybradley It looks like manual action was needed in the rebase, and a marker is still there.

======================================================================
ERROR: Failure: SyntaxError (invalid syntax (test_axes.py, line 1555))
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/virtualenv/python2.7/local/lib/python2.7/site-packages/nose/loader.py", line 402, in loadTestsFromName
module = resolve_name(addr.module)
File "/home/travis/virtualenv/python2.7/local/lib/python2.7/site-packages/nose/util.py", line 311, in resolve_name
module = __import__('.'.join(parts_copy))
File "/home/travis/virtualenv/python2.7/local/lib/python2.7/site-packages/matplotlib-1.4.x-py2.7-linux-x86_64.egg/matplotlib/tests/test_axes.py", line 1555
>>>>>>> added errorbar limit test
^
SyntaxError: invalid syntax

@larrybradley
Copy link
Contributor Author

@efiring I removed the errant marker and rebased again.

@tacaswell
Copy link
Member

Something has gone horribly wrong, there are now many pep8 failures....

@larrybradley
Copy link
Contributor Author

It appears that most are related to comment symbols without spaces immediately afterward, i.e. #comment instead of # comment. The other "unexpected indentation" errors appear to also be related to comment blocks. None of these errors are related to my changes. I double-checked and the offending comments are also in master. Did something possibly change with the pep8 checking?

@tacaswell
Copy link
Member

However, master is currently building cleanly so the issues are arising from this merge. Does it pass pep8 locally for you?

@tacaswell
Copy link
Member

Odd, the pass locally for me, I tried re-starting the travis build.

@tacaswell
Copy link
Member

@larrybradley This needs to be rebased again. I suspect the issue is in CHANGELOG/whats_new

@larrybradley
Copy link
Contributor Author

@tacaswell OK, it's rebased again.

tacaswell added a commit that referenced this pull request Apr 4, 2014
Fixed issues with errorbar limits
@tacaswell tacaswell merged commit 9f28318 into matplotlib:master Apr 4, 2014
@tacaswell
Copy link
Member

merged before this required (yet) another re-base.

@larrybradley larrybradley deleted the limitfix branch April 4, 2014 14:32
@anntzer anntzer mentioned this pull request Aug 12, 2019
6 tasks
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.

None yet

6 participants