Solving crop_by_coord problem when data are rotated #113

BaptistePellorceAstro · 2018-04-18T10:39:18Z

Here a first version to solve the crop_by_coords problem when the data are rotated.

Discussion about it was on issue #99 and issue #100

BaptistePellorceAstro · 2018-04-18T10:44:06Z

ndcube/ndcube.py

@@ -502,7 +537,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.
+        Defaults to None.list


Mistake removed from the file

Cadair · 2018-04-18T11:07:08Z

ndcube/ndcube.py

@@ -85,22 +87,30 @@ def world_axis_physical_types(self):
        pass

    @abc.abstractmethod
-    def crop_by_coords(self, min_coord_values, interval_widths):
+    def crop_by_coords(self, lower_corner, interval_widths=None, upper_corner=None):


An API point here, I think it would be worth moving interval_widths into **kwargs so that it is no longer part of the function signature. We can still support it by doing a kwargs.pop('interval_widths', None) as the ~first line of the method.

Hi, @DanRyanIrish told me to do it like that because some people will use this version as the previous : by inputting corp_by_coords( (low1, low2), (inter1, inter2) ).

If I move interval_widths into **kwargs, I am afraid that people use upper_corner instead of interval_widths and the result will not be correct.

I don't know if you saw it but I add a warning about using interval_widths where I ask the user to use upper_corner.

Of course I am open to every modification and if you want I change it, I'll do it !

ah yes, you are correct. never mind then.

tiagopereira · 2018-04-18T15:41:32Z

Since we are contemplating an API change, can we perhaps consider moving the units into a separate tuple on things like crop_by_coords and world_to_pixel? Maybe it is just me, but the following, taken from the documentation, seems exceedingly verbose and too cumbersome to write on a terminal session:

>>> my_cube_roi = my_cube.crop_by_coords(
... [Quantity(0.7, unit="deg"), Quantity(1.3e-5, unit="deg"), Quantity(1.04e-9, unit="m")],
... [Quantity(0.6, unit="deg"), Quantity(1., unit="deg"), Quantity(0.08e-9, unit="m")])

Why not have the units on a separate argument, e.g.:

>>> my_cube_roi = my_cube.crop_by_coords([0.7, 1.3e5, 1.04e9], [0.6, 1., 0.08e-9], 
...                                      units=["deg", "deg", "m"])

This seems much more readable. If my_cube already has units assigned to each dimension, we can even make the units argument optional and use the units of my_cube by default. We can also add in a check if each item on the list is a Quantity, so people can use the old API if they want to.

Cadair · 2018-04-18T17:11:49Z

@tiagopereira your first example is a little more verbose than it needs be the following would also work:

>>> import astropy.units as u
>>> my_cube_roi = my_cube.crop_by_coords(
... [*(0.7, 1.3e-5)*u.deg, 1.04e-9*u.m],
... [*(0.6, 1.)*u.deg, 0.08e-9*u.m])

I am however not adverse to a units= keyword argument, as long as the units are always explicitly provided by the user. This is the API used by SkyCoord as well, in that if the numbers are provided dimensionless there must be a units= keyword arg that is of the same number of dimensions as the input.

(sorry for accidentally posting and closing the issue lol)

DanRyanIrish · 2018-04-18T20:53:54Z

I believe the current API is better for philosophical reasons, namely that all values of a physical quantity should be represented with units attached. I believe it is also more useful in many cases. However, in the case where the user is entering these values manually on the command line, I agree it does require duplication of the unit definition in the corner arguments. So I am open to discussing whether we support this additional API simultaneous with the current one.

My main concern about this API from a coding point of view, is that it introduces a potentially unnecessary for loop at the start of the method where the Quantity objects are created. This could become a noticeable penalty for high dimensions? Perhaps this is not a major penalty relative to the run time of the rest of the method though. I'm not sure.

I agree with @Cadair that it should not have a default unit, but rather require explicit definition. The default would be derived from the WCS units, which are in SI. These are not the typical units used in solar physics so the majority of solar users will have to explicitly give the units anyway. It also leaves open the chance that users will be confused and frustrated when they enter values in arcsec, nm or erg without defining the units and the cropping happens in deg, m, or J.

DanRyanIrish · 2018-04-18T21:01:41Z

ndcube/ndcube.py

+                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)]


I think it's better if you reverse the order of these if statements. That way you can issue the warning and construct upper_corner from interval_widths without checking the number of elements is right. Then you can just check the length of upper_corner without requiring an if statement since if it wasn't defined by the user, you will have defined in the if interval_widths: if statement.

Then when it comes time to stop supporting the interval_widths API, we just have to delete the if interval_widths: statement.

If i delete the elements number checking between lower_corner, interval_widths and n_dim, an error will come by creating the upper_corner values because one of these tuples will be out of range. Do you want I remove this error raising ?

Ah, you're absolutely right! Yes, leave in the length checking in both statements, but check interval_widths first. Then you can also add in a check that interval_widths and upper_corner were not both set:

# 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 both intervals_widths and upper_corner set, raise error. 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)] # 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.")

Ok, I will input this line in my code, I already exchanged the if statements positions.

DanRyanIrish · 2018-04-18T21:03:23Z

ndcube/ndcube.py

@@ -409,21 +419,46 @@ def extra_coords(self):
                    "value": self._extra_coords_wcs_axis[key]["value"]}
        return result

-    def crop_by_coords(self, min_coord_values, interval_widths):
+    def crop_by_coords(self, lower_corner, interval_widths=None, upper_corner=None):
        # The docstring is defined in NDDataBase

        n_dim = len(self.dimensions)


I know this wasn't your change, but can you replace this is n_dim = self.data.ndim? It's more efficient.

DanRyanIrish · 2018-04-18T22:35:25Z

In fact, I have a suggestion for third API option. @Cadair wont like it because I don't see an easy way to maintain the current API and this one simultaenously. But I think it makes the code more efficient and succinct. It solves @tiagopereira's concern about duplicating the unit definition as my concern about not separating the values from the units. Have the user supply N (number of dimensions) Quantity objects of length 2, each giving the min and max of the region of interest along the each axis. So in @tiagopereira's example this would be:

my_cube_roi = my_cube.crop_by_coords(Quantity([0.7, 1.3], unit="deg"),
                                     Quantity([1.3e-5, 1+1.3e-5], unit="deg"),
                                     Quantity([1.04e-9, 1.12e-9], unit="m"))

This is the same API as with world_to_pixel and pixel_to_world. As well as unifying those APIs, this API for crop_by_coords also means we need fewer for loops through the dimensions by eliminating the need to put the corners args into a format for world_to_pixel. It also means that the unit for each dimension only needs to input once. Here's how I think it should work in code. I'd be interested to hear if @Cadair thinks it really is more efficient though. Let quantity_list be the list of all length 2 Quantity objects input by the user, a min and max for each dimension.

def crop_by_coords(self, *quantity_list):
    # Get all corners of region of interest.  Perhaps there is a tidier/more efficient way of doing this.
    all_world_corners_grid = np.meshgrid(*[axis.value for axis in quantity_list])
    all_world_corners = [all_world_corners_grid[i].flatten()*quantity_list[i].unit 
                         for i in range(n_dim)]
    # Convert to pixel coordinates
    all_pix_corners = self.world_to_pixel(*all_world_corners)
    # Derive slicing item with which to slice NDCube.  
    # Be sure to round down min pixel and round up + 1 the max pixel.
    item = tuple([slice(int(axis_pixels.value.min()), 
                        int(np.ceil(axis_pixels.value.max()))+1) 
                  for axis_pixels in all_pix_corners])
    return self[item]

As you can see, crop_by_coords then becomes a very short method!

Cadair · 2018-04-19T08:50:30Z

@DanRyanIrish you are quite correct I won't like it. However, not really because of the API change.

The two main objections I have are:

It is not the same as SunPy Map. Having changed the API for sunpy map in 0.8 I really don't want to have to change it again.
This API is not conducive to ever moving to allowing rich coordinate objects as inputs. i.e. when the APE 14 stuff has progressed I see this API moving to allowing SkyCoord, Time and Quantity as input, much the same way that Map.submap does in SunPy at the moment.

DanRyanIrish · 2018-04-19T12:31:45Z

The extension to SkyCoord and Time is a good point. In fact, think we can still make the above implementation work without adding an extra loop, while keeping the lower_corner, upper_corner API. See below. It doesn't harmonize the concerns over the unit duplication in the API. But I am open to changing that as a separate issue.

def crop_by_coords(self, lower_corner, upper_corner):
    # Get all corners of region of interest.  Perhaps there is a tidier/more efficient way of doing this?
    all_world_corners_grid = np.meshgrid(
        *[Quantity([lower_corner[i], upper_corner[i]], unit=lower_corner[i].unit).value 
          for i in range(self.data.ndim)])
    all_world_corners = [all_world_corners_grid[i].flatten()*lower_corner[i].unit 
                         for i in range(n_dim)]
    # Convert to pixel coordinates
    all_pix_corners = self.world_to_pixel(*all_world_corners)
    # Derive slicing item with which to slice NDCube.  
    # Be sure to round down min pixel and round up + 1 the max pixel.
    item = tuple([slice(int(axis_pixels.value.min()), 
                        int(np.ceil(axis_pixels.value.max()))+1) 
                  for axis_pixels in all_pix_corners])
    return self[item]

DanRyanIrish · 2018-04-19T13:16:43Z

ndcube/ndcube.py

-        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)])


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.

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.

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.

Ok, I will change that ;)

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.

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.

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.

DanRyanIrish · 2018-04-19T14:04:40Z

Hi @BaptistePellorceAstro. I know we are still discussing implementation and API changes so there is a bit of uncertainty. But irrespective of that, crop_by_coords needs at least one test to check the right answer is obtained in the case where the coordinate system is not aligned with the pixel grid. I suggest this should be the next step in this PR.

The tests for crop_by_coords can be found at https://github.com/sunpy/ndcube/blob/master/ndcube/tests/test_ndcube.py#L816. The way the test works is that the @pytest.mark.parametrize decorator defines the test input and expected output as two tuples. The test function then takes these tuples as input, calculates the result using the test input, and then compares the result with the expected result. So what you need to do is the follow:

Near the top of file, you need to generate an WCS object (see an example here: https://github.com/sunpy/ndcube/blob/master/ndcube/tests/test_ndcube.py#L19) which is misaligned with the pixel grid.
Combine this with a data array and create a new NDCube, called misaligned_cube. See an example here: https://github.com/sunpy/ndcube/blob/master/ndcube/tests/test_ndcube.py#L73. Getting the WCS to misaligned is the tricky part.
Pick a sub region you want from misaligned_cube and work out what the pixel slicing item should.
Add a new tuple to the @pytest.mark.parametrize decorator that gives the test input and expected output in the same way as the existing tuple does.

BaptistePellorceAstro · 2018-04-20T13:06:02Z

Hi, I follow all you ask me to do and now I want to try the test to see if it is working before push my work. can you explain me how to do ?

DanRyanIrish · 2018-04-20T13:24:34Z

Great! Of course. On a terminal change to the top level ndcube directory. It should have a file called setup.py. Once you're there, type on the command line
python setup.py test
This will run all the tests for you and give you information on the ones that fail.

BaptistePellorceAstro · 2018-04-20T13:26:55Z

I succeed to run the test but there is an error with the previous data from the test. I will try to understand why. Do you want I push ?

I used an other command, I will try with yours.

BaptistePellorceAstro · 2018-04-20T13:30:38Z

Ok, I have the same return :
test_crop_by_coords[test_input0-expected0]
1 failed, 257 passed, 3 skipped in 6.49 seconds

So what I understand is that the first tuple of data is failing in the test no ?

DanRyanIrish · 2018-04-20T13:35:28Z

Sure. There's never harm in pushing. The error in the last test may be because you've changed something. So yes, it is important to understand why that is now failing.

pep8speaks · 2018-04-20T13:37:31Z

Hello @BaptistePellorceAstro! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on April 27, 2018 at 09:10 Hours UTC

BaptistePellorceAstro · 2018-04-20T14:31:43Z

I think I have found the problem. They have :
lower_corner = [0.7*u.deg, 1.3e-5*u.deg, 1.02e-9*u.m]
interval_widths = [1*u.deg, 1*u.deg, 1.06*u.m]

Firstly, they ask to crop the X axis from 1nm to 1m. This is a wide crop, so data are not crop in this axis because the data shape in X axis is 4 pixels and values are increasing by step of 0.02 nm per pixel.

Secondly, they ask to crop the Z axis from 0.7 deg to 1.7 deg. This is also a wide crop for the data because the data shape for this axis is only 2 pixels. By step of 0.4 deg as they ask, they are out of range (they need at least 4 pixels). One thing I saw here and on my test, the plotting function seems to be incorrect : in this axis, one pixel is missing in the plot. Do you know why ?

Then, they ask to crop the Y axis from 1.3e-5 deg to 1 deg. This crop is taking 3 pixels because the step is 0.5 deg per pixel. However 3 pixel is the shape of the data in Y axis.

To conclude, this crop is useless and crop nothing from these data. However, they ask a return as cubem[:, :2]. In my mind, there is a mistake in this test. Can you run it with your files to see if you have this bug ? What do you think about this crop ?

DanRyanIrish · 2018-04-20T14:39:05Z

This analysis seems reasonable. Feel free to adjust the inputs to the first test to make a more reasonable check of this method in the case where the coordinate and pixel grids align.

As for the plotting, don't worry about it. There are open PRs which will be merged once sunpy 0.9 is released and will make major changes to the plotting. Energy chasing down that bug at this time probably isn't worth it.

BaptistePellorceAstro · 2018-04-20T14:42:23Z

Nice, I was just wanting to return you that. I will change the first inputs values to solve this test.

DanRyanIrish · 2018-04-20T15:06:54Z

Good job!

BaptistePellorceAstro · 2018-04-23T09:40:29Z

Hi, I have modified the inputs and the output expected for the align grid test. I am limited by selecting a part of data due to a plotting error. As you said before, I don't take care of that and I focused on the smaller part I can select.

BaptistePellorceAstro · 2018-04-25T11:47:36Z

I added the units argument in crop_by_coords, I added some lines and errors raising, and I create new tests.

DanRyanIrish · 2018-04-25T12:36:35Z

ndcube/ndcube.py

+                if type(lower_corner[i]) is not u.quantity.Quantity:
+                    lower_corner[i] = u.Quantity(lower_corner[i], unit=units[i])
+                if type(upper_corner[i]) is not u.quantity.Quantity:
+                    upper_corner[i] = u.Quantity(upper_corner[i], unit=units[i])


You don't have to check the types in this for loop. If the type is already a Quantity the effect will be to convert the Quantity to the given unit. This is probably a more intuitive behaviour if a user gives both Quantities and sets the units kwarg.

Oh, yes, I didn't think that it is working like that ! I go modify that.

DanRyanIrish · 2018-04-25T12:41:28Z

ndcube/ndcube.py

+                if type(lower_corner[i]) is not u.quantity.Quantity or \
+                   type(upper_corner[i]) is not u.quantity.Quantity:
+                    raise ValueError("The inputs are not Quantity objects, you "
+                                     "can use the units argument to set it")


Suggest changing this error message slightly to something like: "lower_corner and interval_widths/upper_corner must be of type astropy.units.Quantity or the units kwarg must be set.".
Alos, the type of error should be TypeError, not ValueError.

DanRyanIrish · 2018-04-25T12:46:15Z

@BaptistePellorceAstro, this is looking very good. I'll have another look at it a bit later. But I think it's getting to be being ready to merge.

DanRyanIrish · 2018-04-25T13:06:27Z

One of the tests is now failing so something needs to be fixed. Have a look at the online test report here: https://travis-ci.org/sunpy/ndcube/jobs/371049917

If you have questions I can answer them a bit later.

Cadair · 2018-04-25T12:28:32Z

ndcube/ndcube.py

+                if type(upper_corner[i]) is not u.quantity.Quantity:
+                    upper_corner[i] = u.Quantity(upper_corner[i], unit=units[i])
+        else:
+            for i in range(n_dim):


It would be better to do this like loop and if statement in one go:

if any([not isinstance(x, u.Quantity) for x in lower_corner + upper_corner]):

Oh nice ! That is what I was looking for, thanks !

Cadair · 2018-04-25T12:30:46Z

ndcube/ndcube.py

+                                 'number of data dimensions.')
+            # If inputs are not Quantity objects, they are modified into specified units
+            for i in range(n_dim):
+                if type(lower_corner[i]) is not u.quantity.Quantity:


we don't need these if statements here just:

for i in range(n_dim): lower_corner[i] = u.Quantity(lower_corner[i], unit=units[i]) upper_corner[i] = u.Quantity(upper_corner[i], unit=units[i])

is sufficient. (Converting units in this case is cleaner and probably faster)

Outdated lines, changes are already made.

DanRyanIrish · 2018-04-26T13:12:21Z

This is looking good @BaptistePellorceAstro. Let us know when you feel it is ready for another review.

BaptistePellorceAstro · 2018-04-26T13:20:16Z

I have finished to work on this version. I am waiting your reviews to continue.

DanRyanIrish · 2018-04-26T14:13:07Z

ndcube/ndcube.py

+        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]) + 1) for i in range(n_dim)])


I think the algorithm from L466 to 483 should be replaced with the following:

# Get all corners of region of interest. all_world_corners_grid = np.meshgrid(*[u.Quantity([lower_corner[i], upper_corner[i]], unit=lower_corner[i].unit).value for i in range(self.data.ndim)]) all_world_corners = [all_world_corners_grid[i].flatten()*lower_corner[i].unit for i in range(n_dim)] # Convert to pixel coordinates all_pix_corners = self.world_to_pixel(*all_world_corners) # Derive slicing item with which to slice NDCube. # Be sure to round down min pixel and round up + 1 the max pixel. item = tuple([slice(int(axis_pixels.value.min()), int(np.ceil(axis_pixels.value.max()))+1) for axis_pixels in all_pix_corners])

This should make the code more efficient as well.

Are your lines checking the boundary conditions of the data ?

Let me know if you have any questions about what this code is doing.

That's an excellent point! If the derived indices are larger than the extent of that axis, that's fine. It'll just return to the maximum extent of the data. But if the pixels are negative that will cause incorrect behaviour.

So we need to ensure the start pixel values in the slice item are non-negative. There's probably a clever way to do this. Let me give it some quick though.

Ok it's clear ! I update and commit again.

Yes! This can be done using np.clip. See the documentation here: https://docs.scipy.org/doc/numpy/reference/generated/numpy.clip.html

So in our case we would do:

item = tuple([slice(int(np.clip(axis_pixels.value.min(), 0, None)), int(np.ceil(axis_pixels.value.max()))+1) for axis_pixels in all_pix_corners])

Oh, your comments just appears, I modify that !

DanRyanIrish · 2018-04-26T14:49:32Z

ndcube/ndcube.py

+        # Get all corners of region of interest.
+        all_world_corners_grid = np.meshgrid(*[u.Quantity([lower_corner[i], upper_corner[i]],
+                                                          unit=lower_corner[i].unit).value
+                                               for i in range(self.data.ndim)])


Suggestion for code readability:

all_world_corners_grid = np.meshgrid( *[u.Quantity([lower_corner[i], upper_corner[i]], unit=lower_corner[i].unit).value for i in range(self.data.ndim)]

It makes the code more readable but minimizing the break up of single items (e.g. Quantities) over multiple lines.

ping @BaptistePellorceAstro. This comment got outdated incorrectly.

Ok I was thinking about it ! I am solving some test bugs from the last commits.

DanRyanIrish · 2018-04-26T16:01:35Z

ndcube/ndcube.py

@@ -1,6 +1,8 @@
 # -*- coding: utf-8 -*-

 import abc
+import warnings
+from itertools import product


Delete this import as it is no longer used.

Oh yeah, I missed this one. I will delete it tomorrow.

DanRyanIrish · 2018-04-26T16:07:29Z

@Cadair, the algorithm in this PR has changed. Any comments?

DanRyanIrish · 2018-04-27T14:11:13Z

Congratulations @BaptistePellorceAstro one another merged PR! 🎉

Cadair · 2018-05-29T12:24:55Z

@DanRyanIrish I assume this should be milestoned 1.0.2 to be released in a bugfix?

DanRyanIrish · 2018-05-29T18:38:46Z

Hi @BaptistePellorceAstro. Yes, that's a good point. I should speak with @Cadair and figure out how to backport the bugfixes, including this one, that we want to release.

DanRyanIrish · 2018-05-29T23:04:41Z

Sorry @Cadair. Misread who this comment was from!!! Yes, we should include it in the next bugfix release.

Cadair · 2018-05-30T10:00:01Z

@DanRyanIrish can we try and make sure that all PRs have a milestone so that we know if they should be backported or not? i.e. we should have a 1.1 and a 1.0.x (currently 1.0.2) milestone for the next bugfix release.

I will backport this.

Solving crop_by_coord problem when data are rotated

Backport #113 to 1.0

DanRyanIrish · 2018-05-30T18:50:44Z

Hi @Cadair. Yes, we can do that. I will go through the PRs when I get a chance and milestone them.

Solving crop_by_coord problem when data are rotated

909ce8c

BaptistePellorceAstro commented Apr 18, 2018

View reviewed changes

Cadair reviewed Apr 18, 2018

View reviewed changes

BaptistePellorceAstro mentioned this pull request Apr 18, 2018

crop_by_coords is broken when data is rotated #100

Closed

Cadair closed this Apr 18, 2018

Cadair reopened this Apr 18, 2018

DanRyanIrish requested changes Apr 18, 2018

View reviewed changes

New error raising and dimension modified

28aba8c

DanRyanIrish reviewed Apr 19, 2018

View reviewed changes

Adding a new test for rotated data

952a712

PEP8 correction in test_ndcube.py

cefd9a7

Solving test bug with old values on crop by coords

5a0f454

DanRyanIrish reviewed Apr 25, 2018

View reviewed changes

Some Quantity types checks refactored

16c08f0

Fix a test error

1adde1d

Cadair reviewed Apr 26, 2018

View reviewed changes

Improving units checks in crop by coords

76501e1

DanRyanIrish requested changes Apr 26, 2018

View reviewed changes

BaptistePellorceAstro added 2 commits April 26, 2018 16:41

Improving corners calculation

7c56239

Improving the data slicing

4f729dd

DanRyanIrish reviewed Apr 26, 2018

View reviewed changes

Fix some error bugs

b097241

DanRyanIrish mentioned this pull request Apr 26, 2018

SJICube Refactor sunpy/sunraster#66

Closed

DanRyanIrish requested changes Apr 26, 2018

View reviewed changes

Removing product importation

d6cf940

DanRyanIrish approved these changes Apr 27, 2018

View reviewed changes

DanRyanIrish merged commit 886f7c1 into sunpy:master Apr 27, 2018

Cadair pushed a commit to Cadair/ndcube that referenced this pull request May 30, 2018

Merge pull request sunpy#113 from BaptistePellorceAstro/master

687e917

Solving crop_by_coord problem when data are rotated

Cadair added a commit that referenced this pull request May 30, 2018

Merge pull request #125 from Cadair/1.0

675235f

Backport #113 to 1.0

DanRyanIrish added this to the 1.0.2 milestone Jun 14, 2018

Solving crop_by_coord problem when data are rotated #113

Solving crop_by_coord problem when data are rotated #113

Conversation

BaptistePellorceAstro commented Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagopereira commented Apr 18, 2018

Cadair commented Apr 18, 2018

DanRyanIrish commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish commented Apr 18, 2018

Cadair commented Apr 19, 2018

DanRyanIrish commented Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish commented Apr 19, 2018

BaptistePellorceAstro commented Apr 20, 2018

DanRyanIrish commented Apr 20, 2018 • edited Loading

BaptistePellorceAstro commented Apr 20, 2018 • edited Loading

BaptistePellorceAstro commented Apr 20, 2018

DanRyanIrish commented Apr 20, 2018

pep8speaks commented Apr 20, 2018 • edited Loading

Comment last updated on April 27, 2018 at 09:10 Hours UTC

BaptistePellorceAstro commented Apr 20, 2018 • edited Loading

DanRyanIrish commented Apr 20, 2018

BaptistePellorceAstro commented Apr 20, 2018

DanRyanIrish commented Apr 20, 2018

BaptistePellorceAstro commented Apr 23, 2018

BaptistePellorceAstro commented Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish commented Apr 25, 2018

DanRyanIrish commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish commented Apr 26, 2018

BaptistePellorceAstro commented Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

BaptistePellorceAstro Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish commented Apr 26, 2018

DanRyanIrish commented Apr 27, 2018

Cadair commented May 29, 2018

DanRyanIrish commented May 29, 2018

DanRyanIrish commented May 29, 2018

Cadair commented May 30, 2018

DanRyanIrish commented May 30, 2018

BaptistePellorceAstro commented Apr 18, 2018 •

edited

Loading

DanRyanIrish commented Apr 19, 2018 •

edited

Loading

DanRyanIrish commented Apr 20, 2018 •

edited

Loading

BaptistePellorceAstro commented Apr 20, 2018 •

edited

Loading

pep8speaks commented Apr 20, 2018 •

edited

Loading

BaptistePellorceAstro commented Apr 20, 2018 •

edited

Loading

BaptistePellorceAstro commented Apr 25, 2018 •

edited

Loading

BaptistePellorceAstro commented Apr 26, 2018 •

edited

Loading

BaptistePellorceAstro Apr 26, 2018 •

edited

Loading