-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Added test for units with Rectangle for PR #5421 #5422
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
Conversation
import matplotlib.testing.jpl_units as U | ||
U.register() | ||
|
||
p = mpatches.Rectangle( ( 5*U.km, 6*U.km ), 1*U.km, 2*U.km ) |
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.
It looks like tests are failing because this line isn't pep8 compliant, could you remove the whitespace from around the opening and closing brackets?
This is a case where someone else taking over the PR is probably the fastest way to get this in. I am confident that @TD22057 would not be offended / would be appreciative. |
FYI I updated the file as requested. The current appveyor failure is caused by some change to the MPL import structure as it worked fine before. Let me know if I have to start over or what to do about this (or just fix it when it's integrated on your end). FYI in case it wasn't clear, the real problem this test shows is issue #5421 which was caused by the changes make in #4751 which fail in this case. I'd like to see this get fixed - we've been hand patching our release of MPL for the last year and half to work around this. |
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.
Mostly unimportant style changes, but two important things do need to be fixed. This should be mergeable after fixing CI.
from __future__ import (absolute_import, division, print_function, | ||
unicode_literals) | ||
|
||
from matplotlib.externals import six |
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.
six
is unused; you can just remove the line (it'll fix the build.)
import numpy as np | ||
from numpy.testing import assert_array_equal | ||
from numpy.testing import assert_equal | ||
from numpy.testing import assert_almost_equal |
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.
All assert_*
are not used; remove the imports.
from numpy.testing import assert_equal | ||
from numpy.testing import assert_almost_equal | ||
|
||
from matplotlib.testing.decorators import image_comparison, cleanup |
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.
cleanup
is not used; remove.
from matplotlib.testing.decorators import image_comparison, cleanup | ||
import matplotlib.pyplot as plt | ||
import matplotlib.patches as mpatches | ||
|
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.
Two blank lines before function.
ax.set_xlim([4*U.km, 7*U.km]) | ||
ax.set_ylim([5*U.km, 9*U.km]) | ||
|
||
if __name__ == '__main__': |
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.
Please remove this entire if
block; it is no longer being used.
@TD22057 Any suggestions on how to fix both the unit issue and the uint8 issue? |
@tacaswell Nope - We've been hand patching MPL to remove the changes made in #4751 for over a year now so that our software will work. I'd suggest writing a unit test for the original problem to make sure whatever the fix is doesn't break that. Then look at what's done in the other artists - they seem to handle this case without any problem. |
Thanks @TD22057 ! Sorry this took so long to be addressed. |
Added a test case and baseline image to create a Rectangle with unit values. Doesn't fix the issue - just demonstrates it.