Creation of IRISMapCubeSequence class #83

BaptistePellorceAstro · 2018-05-07T12:52:41Z

Here the creation of the IRISMapCubeSequence class. See #72 for more informations.

DanRyanIrish · 2018-05-07T13:16:20Z

irispy/new_sji.py

+    """
+    def __init__(self, data_list, meta=None, common_axis=0):
+        # Check that all SJI data are coming from the same OBS ID.
+        if len(np.unique([cube.meta["OBSID"] for cube in data_list])) != 1:


This could be rewritten as:

if any([cube.meta["OBSID"] for cube in data_list] != data_list[0].meta["OBSID"]):

The list might have to be an array. I'm not sure. Check this out.

With this line, I get an error that I solve by doing :

if np.any([cube.meta["OBSID"] != data_list[0].meta["OBSID"] for cube in data_list]):

DanRyanIrish · 2018-05-07T13:19:16Z

irispy/new_sji.py

+                                         extra_coords=extra_coords, scaled=scaled))
+        hdulist.close()
+
+    return IRISMapCubeSequence(list_of_cubes, meta=meta, common_axis=0)


I think an IRISMapCubeSequence should only be returned if there is more than one file input. Otherwise, an IRISMapCube should be returned as before.

Ok, this is boring to use it with only one file (I am working with). I will modify it.

DanRyanIrish · 2018-05-07T14:19:07Z

Thanks for this PR @BaptistePellorceAstro. I made a couple of minor initial comments.

This has brought up a larger issue in my mind. That is, how should the IRISMapCubeSequence behave as far as the user is concerned. In the case of spectrograph images, the sequence dimension has a physical meaning, i.e. the raster scan number. However, in the case of SJI images, the only reason have this sequence dimension is because of how the data is stored in different files. As far as the user cares, the sequence axis and time are axis are just different sized chunks of the same axis. Therefore, perhaps the IRISMapCubeSequence should always appear to the user as 3D.

In principle this could be done by rewriting the IRISMapCubeSequence.__getitem__ to simply call the index_as_cube property. The same is true of the dimensions and world_axis_physical_coords. They can be rewritten to call the cube_like_dimensions and cube_like_world_axis_physical_types. There may be complications with ensuring that we don't end up with circular calling that breaks it. But I'm sure there's a way around it. In fact, a previous implementation had worked this out. But then we removed it.

DanRyanIrish · 2018-05-07T15:30:26Z

irispy/new_sji.py

-    filename : `str`
-        File name to be read
+    filenames: `list` of `str` or `str`
+        Filename of filenames to be read.  They must all be associated with the same


Typo: "of" should be "or"

BaptistePellorceAstro · 2018-05-08T13:00:52Z

Hi, as you said, I have some problems :

dimensions : The problem is that cube_like_dimensions is calling dimensions so I have a circular error. I succeed to solve it by changing the name of my function to dimensions2 for example. Do you have an idea to solve this bug ?
__getitem__ : For every tests I did, I have already this error __getitem__() missing 1 required positional argument: 'item'.

BaptistePellorceAstro · 2018-05-14T12:30:43Z

irispy/new_sji.py

@@ -274,7 +274,7 @@ def __repr__(self):

    def __getitem__(self, item):
        return utils.sequence.slice_sequence(self, item)


I use utils.sequence.slice_sequence(self, item) rather than index_as_cube because the last function is calling a lot of function that are not relevant (creating as many error as functions) and return the same result. What do you think about it ?

I think you should call index_as_cube. The extra checks it does make it more rigorous. We just need to get it working here. And I think simply calling utils.sequence.slice_sequence will not always be correct. The way you should call index_as_cube it is:

return self.index_as_cube[item]

Have you tried this? If so, what errors are you getting?

Now it works !

DanRyanIrish · 2018-05-14T14:36:37Z

Hi @BaptistePellorceAstro. I just noticed how long ago you sent your first comment since I last looked at this. Apologies for the delay in replying. I've left a couple comments to get you going again. I'll also look into the cube_like_dimensions issue you asked about and get back to you.

DanRyanIrish · 2018-05-14T14:44:45Z

@BaptistePellorceAstro, I think the solution to the dimensions issue will require a simple change to NDCubeSequence in the ndcube package:

Create a new hidden method, NDCubeSequence._dimensions, that is just a copy of the current dimensions method.
Rewrite NDCubeSequence.dimensions so it simply calls the hidden version _dimensions.
In NDCubeSequence.cube_like_dimensions replace calls to the self.dimensions method with calls to the hidden version self._dimensions.
Then, back in IRISpy, you should be able to rewrite dimensions with a call to cube_like_dimensions as there will no longer be circular calling.

Does this make sense?

BaptistePellorceAstro · 2018-05-15T11:20:59Z

Thanks for your help, now everything is working. I opened a new PR in ndcube to solve the cycling bug.

DanRyanIrish · 2018-05-15T16:04:28Z

irispy/new_sji.py


    @property
-    def cube_dimensions(self):
+    def dimensions(self):
        return self.cube_like_dimensions

    @property


Sorry, I think I told you to call this property world_axis_physical_coords. It was a typo. It should have the same name as the NDCubeSequence method which is world_axis_physical_types. My mistake. THis means it also needs changing in line 276 well.

DanRyanIrish · 2018-05-15T16:40:45Z

irispy/new_sji.py

@@ -101,11 +103,14 @@ def __repr__(self):
        #Conversion of the end date of OBS
        endobs = self.meta.get("ENDOBS", None)
        endobs = endobs.isoformat() if endobs else None
-        #Conversion of the instance start of OBS
-        instance_start = self.extra_coords["TIME"]["value"][0]
+        #Conversion of the instance start and end of OBS


Just so you know, the convention of comments is to use a space between the # and the comment.

Of course, I just forget to add a space.

DanRyanIrish · 2018-05-15T16:43:08Z

Hi @BaptistePellorceAstro. I think this PR is almost ready to merge! :) I just asked for a method name change that was originally my fault for giving you the wrong name. See above comment.

BaptistePellorceAstro · 2018-05-16T09:16:32Z

If everything is OK for you, I am ready to merge.

DanRyanIrish · 2018-05-16T11:53:31Z

Another merged PR! You're flying through them!

First version of IRISMapCubeSequence

718b467

DanRyanIrish requested changes May 7, 2018

View reviewed changes

Return IRISMapCube or IRISMapCubeSequence

945c9b0

DanRyanIrish requested changes May 7, 2018

View reviewed changes

Some new properties

e613257

BaptistePellorceAstro added 3 commits May 14, 2018 13:56

Rewriting of properties

2b29ab3

Removing commented lines

d36a565

Removing trailing spaces

ccb98fc

BaptistePellorceAstro commented May 14, 2018

View reviewed changes

Bugs fixed on getitem and dimensions

fa0c541

BaptistePellorceAstro mentioned this pull request May 15, 2018

Remove dependence of NDCube.cube_like_dimensions on NDCube.dimensions sunpy/ndcube#121

Merged

DanRyanIrish reviewed May 15, 2018

View reviewed changes

Method name change

757d07c

DanRyanIrish approved these changes May 16, 2018

View reviewed changes

DanRyanIrish merged commit 528efa6 into sunpy:master May 16, 2018

DanRyanIrish mentioned this pull request May 17, 2018

SJICube Refactor #66

Closed

BaptistePellorceAstro deleted the SJICubeSequence branch May 22, 2018 13:19

DanRyanIrish mentioned this pull request May 29, 2018

Concatenate different SJI observations of same OBS ID. #20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creation of IRISMapCubeSequence class #83

Creation of IRISMapCubeSequence class #83

BaptistePellorceAstro commented May 7, 2018 •

edited

Loading

DanRyanIrish May 7, 2018

BaptistePellorceAstro May 7, 2018 •

edited

Loading

DanRyanIrish May 7, 2018

BaptistePellorceAstro May 7, 2018

DanRyanIrish commented May 7, 2018

DanRyanIrish May 7, 2018

BaptistePellorceAstro commented May 8, 2018 •

edited

Loading

BaptistePellorceAstro May 14, 2018

DanRyanIrish May 14, 2018

BaptistePellorceAstro May 15, 2018

DanRyanIrish commented May 14, 2018

DanRyanIrish commented May 14, 2018

BaptistePellorceAstro commented May 15, 2018

DanRyanIrish May 15, 2018

BaptistePellorceAstro May 16, 2018

DanRyanIrish May 15, 2018

BaptistePellorceAstro May 16, 2018

DanRyanIrish commented May 15, 2018

BaptistePellorceAstro commented May 16, 2018

DanRyanIrish commented May 16, 2018

		@@ -274,7 +274,7 @@ def __repr__(self):

		def __getitem__(self, item):
		return utils.sequence.slice_sequence(self, item)

Creation of IRISMapCubeSequence class #83

Creation of IRISMapCubeSequence class #83

Conversation

BaptistePellorceAstro commented May 7, 2018 • edited Loading

Choose a reason for hiding this comment

BaptistePellorceAstro May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish commented May 7, 2018

Choose a reason for hiding this comment

BaptistePellorceAstro commented May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish commented May 14, 2018

DanRyanIrish commented May 14, 2018

BaptistePellorceAstro commented May 15, 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 May 15, 2018

BaptistePellorceAstro commented May 16, 2018

DanRyanIrish commented May 16, 2018

BaptistePellorceAstro commented May 7, 2018 •

edited

Loading

BaptistePellorceAstro May 7, 2018 •

edited

Loading

BaptistePellorceAstro commented May 8, 2018 •

edited

Loading