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.

Sign up for GitHub

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

Solving crop_by_coord problem when data are rotated #113

Merged
merged 14 commits into from
Apr 27, 2018
Merged

Solving crop_by_coord problem when data are rotated #113

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
909ce8c
Solving crop_by_coord problem when data are rotated
BaptistePellorceAstro Apr 18, 2018
28aba8c
New error raising and dimension modified
BaptistePellorceAstro Apr 19, 2018
952a712
Adding a new test for rotated data
BaptistePellorceAstro Apr 20, 2018
cefd9a7
PEP8 correction in test_ndcube.py
BaptistePellorceAstro Apr 20, 2018
5a0f454
Solving test bug with old values on crop by coords
BaptistePellorceAstro Apr 23, 2018
b837d0e
Some error tests added
BaptistePellorceAstro Apr 23, 2018
77b32f7
New API for crop by coords with units
BaptistePellorceAstro Apr 25, 2018
16c08f0
Some Quantity types checks refactored
BaptistePellorceAstro Apr 25, 2018
1adde1d
Fix a test error
BaptistePellorceAstro Apr 25, 2018
76501e1
Improving units checks in crop by coords
BaptistePellorceAstro Apr 26, 2018
7c56239
Improving corners calculation
BaptistePellorceAstro Apr 26, 2018
4f729dd
Improving the data slicing
BaptistePellorceAstro Apr 26, 2018
b097241
Fix some error bugs
BaptistePellorceAstro Apr 26, 2018
d6cf940
Removing product importation
BaptistePellorceAstro Apr 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions ndcube/ndcube.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,24 +422,27 @@ def extra_coords(self):
def crop_by_coords(self, lower_corner, interval_widths=None, upper_corner=None):
# The docstring is defined in NDDataBase

n_dim = len(self.dimensions)
# Raising a value error if the arguments have not the same dimensions.
if upper_corner:
if (len(lower_corner) != len(upper_corner)) or (len(lower_corner) != n_dim):
raise ValueError("lower_corner and upper_corner must have "
"same number of elements as number of data "
"dimensions.")
n_dim = self.data.ndim
# Raising a value error if the arguments have not the same dimensions.
# Calculation of upper_corner with the inputing interval_widths
# This part of the code will be removed in version 2.0
if interval_widths:
warnings.warn("interval_widths will be removed from the API in "
"version 2.0, please use upper_corner argument.")
if upper_corner:
raise ValueError("Only one of interval_widths or upper_corner "
"can be set. Recommend using upper_corner as "
"interval_widths is deprecated.")
if (len(lower_corner) != len(interval_widths)) or (len(lower_corner) != n_dim):
raise ValueError("lower_corner and interval_widths must have "
"same number of elements as number of data "
"dimensions.")
upper_corner = [lower_corner[i] + interval_widths[i] for i in range(n_dim)]
upper_corner = [lower_corner[i] + interval_widths[i]
for i in range(n_dim)]
# Raising a value error if the arguments have not the same dimensions.
if (len(lower_corner) != len(upper_corner)) or (len(lower_corner) != n_dim):
raise ValueError("lower_corner and upper_corner must have same"
"number of elements as number of data dimensions.")
# Derive all corners coordinates
quantity_list = [[lower_corner[i], upper_corner[i]] for i in range(n_dim)]
all_corners = [self.world_to_pixel(*a) for a in product(*quantity_list)]
Expand All @@ -457,8 +460,8 @@ def crop_by_coords(self, lower_corner, interval_widths=None, upper_corner=None):
lower_pixels = corners_array.min(0)
upper_pixels = corners_array.max(0)
# Creating a tuple to crop the data with inputed coordinates
item = tuple([slice(int(lower_pixels[i]), int(upper_pixels[i])) for i in range(n_dim)])

item = tuple([slice(int(lower_pixels[i]), int(upper_pixels[i]))
for i in range(n_dim)])
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, why are you breaking up this line and others like it into two? According to the PEP8 standard, lines of code can be up to 99 characters long. It doesn't affect the functionality. It's just a coding style question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am coding on Atom and I have a line at 70 characters that I have to avoid to exceed.
Then I am coding with three windows on my screen so I only can see around 75 characters. This is better for me. If you want, I can modify it but I think that for user with a lot of code windows, it is hard to see everything.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's general to stick with the PEP8 standard and have lines up to 99 characters. The 70 line limit and size of your screen is specific to you and wont make it the code more readable for others. Not every line has to be 99 characters of course. Split lines where it makes sense. But if one line is less than 99 characters, don't split it over two. As I said, it's more readable and keeps the style standardised with the majority of other Python code out there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will change that ;)

Copy link
Member

Choose a reason for hiding this comment

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

PEP8 says "Limit all lines to a maximum of 79 characters." This is a matter of preference, but I think it is a bad idea to go over 80. If the python standard library can make it under 80 characters, I don't see why we shouldn't be able to. The vertical line in Atom should default to 80.

Copy link
Member

Choose a reason for hiding this comment

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

The general policy in SunPy core is 99 since they added this to PEP8:

For code maintained exclusively or primarily by a team that can reach agreement on this issue, it is okay to increase the nominal line length from 80 to 100 characters (effectively increasing the maximum length to 99 characters), provided that comments and docstrings are still wrapped at 72 characters.

I leave the decision up to @DanRyanIrish though.

Copy link
Member

Choose a reason for hiding this comment

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

Let's stick with the SunPy core policy of 99 character-long lines. I believe the 80 character limit comes from the days when computers were operated with punch cards! Nowadays most people have big and/or multiple screens so viewing 99 characters isn't a problem. It also encourages (or doesn't inhibit) the use of longer, more explicit variable names which is a good thing. I find that this and reducing cases where code is split over multiple line makes code far more readable. And I'm sure you can change the right margin of editor's like Atom to 100 characters.

return self[item]

def crop_by_extra_coord(self, min_coord_value, interval_width, coord_name):
Expand Down Expand Up @@ -537,7 +540,7 @@ class NDCubeOrdered(NDCube):
for standard deviation or "var" for variance. A metaclass defining
such an interface is NDUncertainty - but isn’t mandatory. If the uncertainty
has no such attribute the uncertainty is stored as UnknownUncertainty.
Defaults to None.list
Defaults to None.

mask : any type, optional
Mask for the dataset. Masks should follow the numpy convention
Expand Down