Add Real World Coordinates from Sliced Out Axes of WCS to Extra_coords

[kakirastern]

To follow up on Issue #136 to add a new feature.

[pep8speaks]

Hello @kakirastern! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-08-19 19:43:40 UTC

[DanRyanIrish]

Hi @kakirastern. Thanks for this PR. I'm looking forward to having this issue addressed.

Before giving you further advice on how best to move ahead, I think it would be instructive if you told me a little of how you understand _wcs_slicer works. (It took me a little while to reteach myself!) Then we can move ahead from that point more effectively.

[kakirastern]

Sure @DanRyanIrish. So the _wcs_slicer as defined in the file wcs.py allows slicing to be done in three possible ways using the slice() function of Python, which is screened as a slice by the isinstance() function:

1. Normal slicing - with slicing indicated in full
2. Slicing indicated by an integer
3. Slicing indicated by a tuple

As such three main conditional statements have been used in the definition, to first decide which kind of slicing (of the above three) has been applied, then to check each sliced item to be added to the output variable new_wcs following the flow according to the manner in which the wcs is sliced. The _wcs_slicer also checks for missing_axis, and modifies the wcs if one such axis is indeed missing. What I suggest doing is to divert the sliced out or dropped wcs "items" to a new variable called dropped_wcs, which hails from the collected list named dropped_list. So whenever items are not appended to the item_checked list, I re-direct these to the dropped_list. I might have unintentionally oversimplified things a little, as I have yet to run the code to test it. But the basic ideas have been captured in the PR. If that is the case please do let me know. I will try my best to make things right as mistakes arise.

[DanRyanIrish]

That's OK @kakirastern. Seems you have a fairly good understanding, especially considering that this overall slicing infrastructure this sits in is not the simplest. One further question. What is your understanding of the purpose of the missing_axis variable?

[kakirastern]

Thanks, @DanRyanIrish. The missing_axis attribute is an additional feature added to the WCS object (NDCube.wcs.missing_axis) to enable it to be sliced seamlessly with NDCube. This is because the WCS object might have missing spatial axis/axes in the data, such as the longitude and latitude pair that is expected to go together in a 3D datacube. In cases where only one spatial dimension is present, as in the case of a slit-spectrograph, NDCube supports the slicing of such WCS objects by creating a tuple in place of the "dead" or missing axis. Without this attribute, the extraction of the spatial axes would break the translation. A more comprehensive definition of the missing_axis variable can be found here. Putting it briefly, the variable is a list of boolean values: If the axis is missing in the data, then its corresponding value in the variable is set to True, otherwise it is assigned a value of False.

[DanRyanIrish]

You really seem to have got your head around it!

In that case, let's look more closely at your PR. The function _wcs_slicer takes an item supplied by the user as part of the standard slicing API applied to an NDCube. It converts the item, which has several valid forms as you listed above, to a standard format so that we can inspect it in a simple way and infer things, such as whether an axis becomes a "missing" axis.

However if I remember correctly, an axis whose data dimension is sliced away is only retained and labelled "missing" if a non-missing axis is dependent on it, like in the case of your latitude/longitude example. If this is not the case, the axis is dropped altogether from the WCS, as is its corresponding entry in missing_axis. Therefore, we may not be able to reliably use missing_axis to achieve our goal here.

Looking more closely at _wcs_slicer, each branch of the if statement produces a new_wcs variable which is sliced using item_checked. Since this step is done in every branch of the if statement, I suggest that we instead only produce a commonly formatted item object within the if statement (item_ = item_checked, item_ = (item_checked) or item_ = _slice_list(item_checked) depending on the branch of the if statement) and leave the definition of new_wcs for after the if statement.

This means we can both slice the new WCS (new_wcs = wcs.slice(item_)) and inspect the item_ in single location after the if statements. This will allow us to search for slice objects in the item_ which will cause an axis to be dropped, rather than doing it in three locations throughout the if statement. Once we identify those dropped axes, we can retrieve the axis name and the value at the location along the axis where the slice is taken, using the wcs.all_pix2world method of the old wcs.

Finally, instead of returning a dropped_axis list, we can return an iterable of tuples or a dictionary or whatever which matches the dropped axis name with the value. We can then insert these "extra_coords" (we define them as such because they are no longer described by the WCS) to NDCube._extra_coords upstream in the slicing infrastructure.

I suggest we start doing this only for non-dependent or dropped sets of dependent axes. This should hopefully make the process a little easier. And we may want to keep the dependent axes values only available in the WCS...perhaps? That is as yet an open question.

How does this sound as a strategy? I realise I've written a lot here and it may not all be clear. Feel free to respond with questions about any of it.

[kakirastern]

Yup, I agree with your suggested approach to adding the new feature by way of a a commonly formatted item object in the three main branches of the _wcs_slicer code (item_ = item_checked, item_ = (item_checked) or item_ = _slice_list(item_checked)), to be followed by a single definition of new_wcs for after the if statement has ended. So I expect much of the definition for _wcs_slicer has to be rewritten... I will need some time to study this and will get back to you soon.

[DanRyanIrish]

Hi @kakirastern. No I think it requires rewriting of _wcs_slicer. Basically just moving the new_wcs definition outside the if statements and make sure that item_ has the same form no matter what if branch the code uses. This is almost the case already.

The biggest effort will be adding a piece of code to _wcs_slicer that checks of slice items whose start and stop parameters differ by 1, then pulling out the real world coordinate for that slice and adding to the extra_coords variable that needs creating.

[kakirastern]

HI @DanRyanIrish. Sure, I have just pushed the first (or the easy) part of the changes to my branch... Am thinking about the second part of your comments though. Will need a bit of time to think things through... Really appreciate your comments as a guide! They are really helpful.

[DanRyanIrish]

These changes look good @kakirastern. I think you're on the right track.

[kakirastern]

I just figured out the next few lines of code to add...

if isinstance(item_, slice):
    start = item_.__getattribute__('start')
    stop = item_.__getattribute__('stop')
    if (stop - start) == 1:

Will need time to think about the rest of the bits to be added. Think this is any good?

[DanRyanIrish]

Hi @kakirastern. I think item_ should be an iterable of slices, one slice for each data dimension of the cube. In that case you would iterate through each data dimension and perform your check within the ``for``` loop.

So you should play around with _wcs_slicer to confirm that item_ does always take that form. If not, the code in the large if statement should be altered slightly to item_ is indeed always an iterable of slices.

One other piece of advice which will be very helpful is to write unique commit messages. They don't have to be long. But something that will give readers like me an idea about what changes were made and why is very useful. The files you changed is easily inspected on GitHub, so "Update wcs.py" doesn't tell us anything useful. Just some helpful advice in moving forward with your git career :)

[kakirastern]

Thanks @DanRyanIrish for the advice about commenting on PR! So I will just use the following instead:

for slice_element in item_:
    if isinstance(slice_element, slice):
        start = slice_element.__getattribute__('start')
        stop = slice_element.__getattribute__('stop')
        if (stop - start) == 1:
            ...

Still thinking about the ... part. Give me say a couple days to make sure.

[DanRyanIrish]

Right. Just from a code point of view, I think all elements in item_ should be slice objects so no need to check here. Also, I think you can be a little less verbose by simply doing:

for slice_element in item_:
    if (slice_element.stop - slice_element.start) == 1:
        ...

[kakirastern]

Thanks for the suggestion!
I have just made some changes to the PR, but think it is not entirely functional yet... May need some tweaks.

[DanRyanIrish]

Hi @kakirastern. Apologies for the delay in responding. I was preoccupied with the holiday weekend in the States. What country/timezone are you in by the way?

Anyway, I've made some suggestions on how to move ahead with your PR. You're definitely moving in the right direction.

[kakirastern]

No worries @DanRyanIrish. I am from Hong Kong by the way, so am in the GMT+8 time zone. Thanks for checking the code I am modifying in this PR and for your instructions/suggestions. I have tried hard to follow them. Please see my attempts.

[kakirastern]

@DanRyanIrish It's weird, seems like some changes have not been saved since the last time I tried to change them. Might be why it appears somewhat odd, especially after you already made some request for me to modify some codes... Let me check what I have done wrong.

[DanRyanIrish]

Hmmm. Have you committed and pushed your latest changes to your github account?

[kakirastern]

I checked with my branch... I think I may have been careless about some of the changes, but most others have been committed and pushed to my branch. @DanRyanIrish, I am sorry about the confusion earlier.

[DanRyanIrish]

No worries about the confusion. It's easy to make mistakes with git when you're starting out...sometimes even when you're more experienced too!

[DanRyanIrish]

Hi @kakirastern. I left a few minor comments but left didn't comment on the innermost loop where the extra coords are compiled as you seem to still be working on that. Let me know when you're stuck/ready for more comments on that part.

In working these things out, I would recommend building your own WCS object following the ndcube docs (http://docs.sunpy.org/projects/ndcube/en/stable/ndcube.html) and play around with the commands and objects from the code on the command line/jupyter notebook to better understand what's going on. Perhaps you are doing this already though.

[DanRyanIrish]

I'm liking the new git commit comments, by the way! ;)

[kakirastern]

Got rid of all the bugs in test_ndcube.py.

[kakirastern]

I will try to see where things are after the latest NDCube release and go from there.

[DanRyanIrish]

Hi @kakirastern. Just to let you know that I took a look at this last night. Because of some significant changes to the code base, this will likely need a serious rebase. This effort will have to be a longer term goal than we originally hoped and so no further work should be done on it for now. However, I think keeping this PR open as it will be very helpful to that future effort as a reminder the desired behaviour.

[kakirastern]

Hi @DanRyanIrish Sure

[stale]

This pull request has been automatically marked as stale because it has not had any activity for the past five months. It will be closed if no further activity occurs. If the ideas in this pull request are still worth implementing, please make sure you open an issue to keep track of that idea!

[stale]

This pull request has been automatically closed since there was no activity for a month since it was marked as stale. If the ideas in this pull request are still worth implementing, please make sure you open an issue to keep track of that idea!