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

Configure + enable stickler testing. #970

Merged
merged 3 commits into from
Nov 30, 2017
Merged

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Nov 28, 2017

No description provided.

@@ -746,7 +747,6 @@ def quick_vertices_transform(self, vertices, src_crs):
mod = np.diff(src_crs.x_limits)[0]
bboxes, proj_offset = self._bbox_and_offset(src_crs)
x_lim = xs.min(), xs.max()
y_lim = ys.min(), ys.max()
for poly in bboxes:
Copy link
Member

Choose a reason for hiding this comment

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

@pp-mo What's happened to y_lim?

Copy link
Member Author

Choose a reason for hiding this comment

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

'y_lim' was not used, so flake8 complained about it, so I got rid of it.

There's a lot of that going on in this PR !

@@ -1184,7 +1184,6 @@ def __init__(self, central_longitude=0.0, central_latitude=0.0,
globe=globe)

a = np.float(self.globe.semimajor_axis or WGS84_SEMIMAJOR_AXIS)
b = np.float(self.globe.semiminor_axis or WGS84_SEMIMINOR_AXIS)
lon, lat = central_longitude + 180, - central_latitude + 0.01
Copy link
Member

Choose a reason for hiding this comment

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

@pp-mo And also what's happened to b (the semiminor axis)?

Copy link
Member Author

Choose a reason for hiding this comment

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

'b' also not used anywhere.
As it was identical, I suspect this one was a mistake from the first.

Copy link
Member

Choose a reason for hiding this comment

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

Identical to what?

Copy link
Member Author

Choose a reason for hiding this comment

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

Identical to 'a'.
If it was going to be useful, I'd expect it to be different in some way, but it's all just guesswork.

Copy link
Member

Choose a reason for hiding this comment

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

It's not identical to 'a', though. But I'm not sure it needs to be used anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see, it is not after all : more care required !

@@ -108,4 +108,4 @@
# Commonly used sub-modules. Imported here to provide end-user
# convenience.
import cartopy.crs
import cartopy.feature
import cartopy.feature # noqa: F401 (imported but not used)
Copy link
Member

Choose a reason for hiding this comment

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

Does this comment stop Stickler complaining?

Copy link
Member Author

@pp-mo pp-mo Nov 29, 2017

Choose a reason for hiding this comment

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

Yes, it's a specific indication to the flake8 tester, actually the "pyflakes" component
The bit in brackets is just comment to the reader.

If anyone is interested ...
It's really not very clearly explained, but after a bit of digging http://flake8.pycqa.org/en/latest/user/violations.html
So, "F" numbers are errors from pyflakes, and "E" and "W" numbers are from pycodestyle (pep8-successor).

I think I'll edit to make this clearer (but it ain't always easy getting it all on one line, which seems to be required)

Copy link
Member

@corinnebosley corinnebosley left a comment

Choose a reason for hiding this comment

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

I'm happy with all these changes. Seems fine to me. Good job @pp-mo

l = list(zip(np.zeros(n_steps) + x,
np.linspace(min(y_ticks), max(y_ticks), n_steps)))
lines.append(l)
ticks = list(zip(
Copy link
Member

Choose a reason for hiding this comment

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

I could've sworn I'd written some optimization for this unfortunate type conversion; maybe I never made a PR out of it, but I can see if I can dig it up.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see a need to connect this with the issue at hand ?
We rather want to get this onto master so we can get on right now, since stickler is now complaining about various open PRs.

Copy link
Member

Choose a reason for hiding this comment

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

No, just a reminder to myself.

@@ -249,7 +249,7 @@ def test_pcolormesh_global_with_wrap1():
@ImageTesting(['pcolormesh_global_wrap2'])
def test_pcolormesh_global_with_wrap2():
# make up some realistic data with bounds (such as data from the UM)
nx, ny = 36, 18
nx = 36
xbnds, xstep = np.linspace(0, 360, nx - 1, retstep=True, endpoint=True)
ybnds, ystep = np.linspace(-90, 90, nx - 1, retstep=True, endpoint=True)
Copy link
Member

Choose a reason for hiding this comment

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

Seems better to make this ny (but that would require changing ny since it was not used before.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Really probably just n, but I don’t think it’s really worth worrying about.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, yes I hadn't spotted it -- it looks like that was the error.
Fixed..

Copy link
Contributor

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Looks clean to me.

@@ -249,7 +249,7 @@ def test_pcolormesh_global_with_wrap1():
@ImageTesting(['pcolormesh_global_wrap2'])
def test_pcolormesh_global_with_wrap2():
# make up some realistic data with bounds (such as data from the UM)
nx, ny = 36, 18
nx = 36
xbnds, xstep = np.linspace(0, 360, nx - 1, retstep=True, endpoint=True)
ybnds, ystep = np.linspace(-90, 90, nx - 1, retstep=True, endpoint=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Really probably just n, but I don’t think it’s really worth worrying about.

@pp-mo
Copy link
Member Author

pp-mo commented Nov 29, 2017

Hi @QuLogic if we have your interest, would you consider merging this ?
If not, fine, but maybe you just sign off your look at it.
Then I'm happy to merge myself, as I think it's had enough other eyes now.

@pp-mo pp-mo merged commit c572fca into SciTools:master Nov 30, 2017
@pp-mo pp-mo deleted the flake_working branch November 30, 2017 10:08
@QuLogic QuLogic added this to the 0.16 milestone Nov 30, 2017
strezen pushed a commit to strezen/cartopy that referenced this pull request Feb 9, 2018
* Configure + enable stickler testing with flake8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants