-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Remove unnecessary calls to float() before division. #10331
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ | |
colors = prop_cycle.by_key()['color'] | ||
|
||
lwbase = plt.rcParams['lines.linewidth'] | ||
thin = float('%.1f' % (lwbase / 2)) | ||
thin = lwbase / 2 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By curiosity, was there a purpose to the former "rounding to 0.1" scheme? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably to make the title label look better(?), but I think 0.75 looks just fine too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough, and I think the same |
||
thick = lwbase * 3 | ||
|
||
fig, axs = plt.subplots(nrows=2, ncols=2, sharex=True, sharey=True) | ||
|
@@ -29,7 +29,7 @@ | |
|
||
axs[1, icol].set_facecolor('k') | ||
axs[1, icol].xaxis.set_ticks(np.arange(0, 10, 2)) | ||
axs[0, icol].set_title('line widths (pts): %.1f, %.1f' % (lwx, lwy), | ||
axs[0, icol].set_title('line widths (pts): %g, %g' % (lwx, lwy), | ||
fontsize='medium') | ||
|
||
for irow in range(2): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -367,12 +367,8 @@ def calculate_rms(expectedImage, actualImage): | |
raise ImageComparisonFailure( | ||
"Image sizes do not match expected size: {0} " | ||
"actual size {1}".format(expectedImage.shape, actualImage.shape)) | ||
num_values = expectedImage.size | ||
abs_diff_image = abs(expectedImage - actualImage) | ||
histogram = np.bincount(abs_diff_image.ravel(), minlength=256) | ||
sum_of_squares = np.sum(histogram * np.arange(len(histogram)) ** 2) | ||
rms = np.sqrt(float(sum_of_squares) / num_values) | ||
return rms | ||
# Convert to float to avoid overflowing finite integer types. | ||
return np.sqrt(((expectedImage - actualImage).astype(float) ** 2).mean()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is seems to be a substantive change to a genuine RMS, from something I don't understand. There must have been a reason for the convoluted original. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand the original either, but the relevant tests (which check for exact return values of this function) are still passing... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ours are, but I suspect this might cause problems for packagers who put in larger tolerances than ours, as I believe some do. Therefore I recommend sticking to the original algorithm for the purposes of this PR. Perhaps add a comment that this is not a simple rms. @mdboom can probably tell us the rationale for the algorithm. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT the results should be the same: the old method counted, for each delta, the number of pixels that have this delta value, then computed a RMS based on that histogram. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I am correctly expressing the two algorithms below, then their results are not the same:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the previous algorithm was dividing by num_values (so sum() / len(x) in your case, not mean()). this fix shows that the values are indentical. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops... sorry I missed that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I can run the Cartopy test suite without any issue using this PR. |
||
|
||
|
||
def compare_images(expected, actual, tol, in_decorator=False): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1573,7 +1573,7 @@ def tick_values(self, vmin, vmax): | |
""" | ||
if self.nbins is None: | ||
return self.locs | ||
step = max(int(0.99 + len(self.locs) / float(self.nbins)), 1) | ||
step = max(int(np.ceil(len(self.locs) / self.nbins)), 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't this change the results in rare cases? It does look nicer, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll go along with this one. |
||
ticks = self.locs[::step] | ||
for i in range(1, step): | ||
ticks1 = self.locs[i::step] | ||
|
@@ -2323,7 +2323,7 @@ def get_log_range(lo, hi): | |
total_ticks = (a_range[1] - a_range[0]) + (c_range[1] - c_range[0]) | ||
if has_b: | ||
total_ticks += 1 | ||
stride = max(np.floor(float(total_ticks) / (self.numticks - 1)), 1) | ||
stride = max(total_ticks // (self.numticks - 1), 1) | ||
|
||
decades = [] | ||
if has_a: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -268,7 +268,7 @@ def from_any(size, fraction_ref=None): | |
return Fixed(size) | ||
elif isinstance(size, six.string_types): | ||
if size[-1] == "%": | ||
return Fraction(float(size[:-1])/100., fraction_ref) | ||
return Fraction(float(size[:-1]) / 100, fraction_ref) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because the purpose here is to convert There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. Sorry, I didn't pay enough attention to the context. |
||
|
||
raise ValueError("Unknown format") | ||
|
||
|
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.
Why did you remove the division by 255?
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.
because we can display uint8 images just as well as float images, and the real point of the example is to show how to get the mathtext as an image array...