Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
MAINT: Updated tick and category test formatting#7993
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lib/matplotlib/tests/test_ticker.py Outdated
| assertformatter.format_pct(x,display_range)==expected | ||
| classTestEndFormatter(object): |
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.
typo: EngFormatter
| np.array(['1','11','3']), | ||
| [b'1',b'11',b'3'], | ||
| np.array([b'1',b'11',b'3'])], | ||
| ] |
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.
apparently an extra ] here messing up travis?
story645 commentedJan 30, 2017
😄 thanks! |
madphysicist commentedJan 30, 2017
Thanks for looking through the PRs. I fixed up the issues you found. |
madphysicist commentedJan 30, 2017
I am pretty sure that this PR is ready to go. The failing tests appear to be caused by an external source. |
6d2c93f to37592a7Comparemadphysicist commentedJan 31, 2017
No more failing tests. |
f4b6f48 to37592a7Comparemadphysicist commentedJan 31, 2017
@dstansby. I've been seeing [MRG+1] a lot recently on PR titles. What does it mean? |
story645 commentedJan 31, 2017
It means your PR has been approved by one reviewer (matplotlib asks for 2 approvals before commiting).http://matplotlib.org/devel/coding_guide.html#pr-review-guidelines |
madphysicist commentedJan 31, 2017
Can you be the other one? |
lib/matplotlib/tests/test_ticker.py Outdated
| cases, especially the case when no SI prefix is present, for values in | ||
| [1, 1000). | ||
| Should not raise exceptions. |
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.
don't need this comment.
lib/matplotlib/tests/test_ticker.py Outdated
| Should not raise exceptions. | ||
| """ | ||
| unitless=mticker.EngFormatter() |
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.
these should probably be two separate tests (unitless and with unit) and possibly parameterized
story645 commentedJan 31, 2017
Sorry thought I approved. Have two comments, but if you wanna punt those to a different improve engineering test PR, let me know and I'll merge. |
9d8d3ab toe3f721fComparemadphysicist commentedJan 31, 2017
@story645 I made the improvements here. I kept is as a single parametrized test, removed the extra comment and also took care of some extra whitespace in ticker.py. |
85ee55b to5211dc9Comparemadphysicist commentedFeb 1, 2017
Not an actual failure, Mac test just didn't run. |
QuLogic commentedFeb 1, 2017
With#7973 merged, this has conflicts. When you rebase, please be sure to leave out any empty |
story645 commentedFeb 1, 2017
I think on the rebase or something, you accidentally introduced a conflict. Please resolve. |
madphysicist commentedFeb 1, 2017
@QuLogic Should I replace |
QuLogic commentedFeb 1, 2017
Classic is the default now, it's not necessary if that's really what's being used (and I realize I forgot to remove a couple.) |
madphysicist commentedFeb 1, 2017
In that case I will remove those too. |
madphysicist commentedFeb 1, 2017
Just to make triple sure, I should make the replacement for |
QuLogic commentedFeb 1, 2017
That's correct. |
5211dc9 tof9f2cb1Comparemadphysicist commentedFeb 1, 2017
Done. I ended up parametrizing a couple of the tests and adding one |
madphysicist commentedFeb 1, 2017
Again looks like Mac test didn't run. No actual failures. |
madphysicist commentedFeb 2, 2017
I've closed and reopened the PR to restart Travis and hopefully get the Mac test to run, but if you approve, this PR is ready to go. |
lib/matplotlib/tests/test_ticker.py Outdated
| 9.441,12.588]) | ||
| assert_almost_equal(loc.tick_values(-7,10),test_value) | ||
| deftest_MultipleLocator_set_params(self): |
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.
Seems like you forgot to remove the class in this one.
lib/matplotlib/tests/test_ticker.py Outdated
| fmt=ax.xaxis.get_major_formatter() | ||
| fmt.set_locs(ax.xaxis.get_majorticklocs()) | ||
| show_major_labels= [fmt(x)!=''forxin | ||
| ax.xaxis.get_majorticklocs()] |
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.
Not aligned properly; better to wrap atfor.
lib/matplotlib/tests/test_ticker.py Outdated
| @pytest.mark.parametrize( | ||
| 'labelOnlyBase, exponent, locs, positions, expected',param_data) | ||
| @pytest.mark.parametrize('base',base_data) | ||
| deftest_LogFormatterExponent(self,labelOnlyBase,base,exponent, |
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.
Class name is still here.
lib/matplotlib/tests/test_ticker.py Outdated
| classTestLogFormatterSciNotation(object): | ||
| testdata= [ |
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.
Whytestdata and nottest_data like the others?
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 wasn't sure if it would cause any problems if I started it withtest_.
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.
We'll see how the tests go, then, but I'd guess this shouldn't be a problem since it isn't callable.
f9f2cb1 to3109ce9Comparemadphysicist commentedFeb 2, 2017
@QuLogic Made the following changes:
|
madphysicist commentedFeb 2, 2017
Ready to go? |
Uh oh!
There was an error while loading.Please reload this page.
Rearranged tests in
lib/matplotlib/tests/test_ticker.pyand made a couple of very minor changes tolib/matplotlib/tests/test_category.py. Based on comment by@story645 in PRs#7482 and#7965.This PR should probably be merged before either of the others so I can rebase them onto it and clean things up.