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

SJICube Refactor #66

Closed
DanRyanIrish opened this issue Mar 14, 2018 · 9 comments
Closed

SJICube Refactor #66

DanRyanIrish opened this issue Mar 14, 2018 · 9 comments
Milestone

Comments

@DanRyanIrish
Copy link
Member

DanRyanIrish commented Mar 14, 2018

SJICube was the first working data object in IRISpy and played an important role in proving that Python could be used to handle IRIS data well. Since then, however, there has been a lot of work done and SJICube is now due a refactor.

SJICube should be rewritten to inherit from ndcube.NDCube and many of the attributes should be pruned. In order to start the discussion on this topic I will outline some suggestion for the refactor.

  • SJICube should be a subclass of ndcube.NDCube, thereby inheriting its slicing, plotting, WCS, extra coordinate, etc. capabilities.
  • SJIMap should be removed as it will no longer be needed if SJICube inherits from ndcube.NDCube.
  • SJICube's initialization should not depend on FITS but rather take SJI data already put into Python objects like numpy arrays. A separate FITS SJI reader function should then be written to take data from the file and put it into an SJICube. This would make SJICube independent of the file type and make it more flexible for users and testing.
  • The majority of the properties for accessing the metadata should be removed. The metadata can then simply be stored in the .meta dictionary. However, some methods should remain, e.g. .draw_slit(). Further discussion is required to decide which should go and which should stay.

Since opening this issue, @BaptistePellorceAstro has been working to refactor SJICube has has/will pursue the following workflow:

Completed

To Do

For extra optional credit (challenging):

@DanRyanIrish
Copy link
Member Author

DanRyanIrish commented Mar 16, 2018

@BaptistePellorceAstro: I think the first stage of this effort is to write a reader function to read in the SJI data into an NDCube object. This could be put into the sji.py file. Then once that's working we can discuss the methods and metadata we want attached to the object (e.g. determining and storing the correct colour table based on the SJI bandpass) and that'll form the basis for the SJICube object.

@DanRyanIrish
Copy link
Member Author

@BaptistePellorceAstro, well done for writing the new SJI reader function in PR #67.

I think the next step should be create a new SJICube class. For now let's call it _SJICube so we can develop, play, and merge it with it without overwriting the existing SJICube. We can do that once we've got _SJICube to a usable state.

For the first version of _SJICube, it should inherit all the attributes of NDCube and add/overwrite just a few methods. To inherit from NDCube, define _SJICube like so:

class _SJICube(NDCube):
    etc. etc.

The methods that you should include explicitly include in _SJICube at the moment are:

Then I think it would be a good time to have a conversation about what else is needed of _SJICube and how to achieve that.

@tiagopereira
Copy link
Member

@DanRyanIrish, I had a chat with Baptiste about draw_slit. We should better define the purpose of this method and when it should be called.

If I understand correctly, under the old SJIMap only one timestep was shown, and therefore draw_slit made more sense. However, in the new SJICube we have a time sequence. The plot object is also no longer a simple plot but a WCS animation. One can call it with a boolean argument (draw_slit(True)) and then the .plot() method would have to know how to plot the slit when called, updating the location with timestep. This would avoid the ambiguity of what to do if draw_slit is called and there is no text.

A related question: is there an axvline equivalent that works with Quantity instead of pixels for WCSAxes?

@DanRyanIrish
Copy link
Member Author

DanRyanIrish commented Apr 4, 2018

Hi @tiagopereira. Yes you're right. draw_slit will need to be folded into the SJICube.plot method. I think it's fine to leave it out of the next PR and just focus on the __repr__. I think a bigger priority for SJICube is to fix the bug in NDCube.crop_by_coods. But we should also have a call and discuss what needs to go into SJICube.

Regarding axvline for Quantities, I'm not sure. Perhaps WCSAxes does this by default? But I believe axvline works on the axis values, rather than the indices. So if WCSAxes doesn't automatically recognise equivalencies, it's an easy matter to convert the Quantity you want to use in axvline to the same unit as the axes.

@Cadair
Copy link
Member

Cadair commented Apr 4, 2018

There isn't an equivalent of ax[h/v]line but there is http://docs.astropy.org/en/stable/api/astropy.visualization.wcsaxes.WCSAxes.html#astropy.visualization.wcsaxes.WCSAxes.plot_coord, which is probably your best bet.

@DanRyanIrish
Copy link
Member Author

Now that @BaptistePellorceAstro is making great progress, I'd like to suggest revisit the roadmap agreed upon at the top of this issue. I have thought of two very important features that I think are very important for handling SJI and IRIS data at large. Depending on how much time @BaptistePellorceAstro has to go in his internship, we should consider working them into our roadmap.

First, SJICube cannot handle data from multiple files. Nor can it break up the frame in one SJICube into multiple SJICubes, each with its own WCS. This is important if fine-scale pointing corrections need to be made on a frame by frame basis. To add this capability, we need to develop a new class, SJICubeSequence, which inherits from NDCubeSequence. I will raise a separate issue outlining the details of how it should work.

Second, in order to facilitate going from an SJICube to an SJICubeSequence, with each image as its own 2D SJICube, we will need to develop an explode_along_axis method for NDCube, similar to the one that already exists in NDCubeSequence.

Thirdly, IRISpy desperately needs the ability to return the time-dependent response function. It can currently get the pre-launch response function but cannot do the necessary fitting/interpolation to get it based on a date. People at LMSAL said they would do this but there has been no progress for months. I'll look into whether work is in fact still going on. If not, we should consider adding this to our roadmap.

I believe that these three actions items are more important than those still on our roadmap with the exception of the dust mask.

@BaptistePellorceAstro
Copy link
Contributor

Nice ! My internship will end at the end of June so I did the first half of my internship. Tell me on what you want I work and I will doing it ! I will take a view on spectrograph.py to see how NDCubeSequence is used.

@DanRyanIrish
Copy link
Member Author

Sounds like you're enthusiastic for this way forwards, @BaptistePellorceAstro! Great. I'll alter the roadmap at the top of this issue accordingly and also inquire about what if any work has been done on the response function fitting.

@DanRyanIrish
Copy link
Member Author

Roadmap updated.

@DanRyanIrish DanRyanIrish added this to the 0.1 milestone Jun 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants