search

sunpy / ndcube

A base package for multi-dimensional contiguous and non-contiguous coordinate-aware arrays.
http://docs.sunpy.org/projects/ndcube/
BSD 2-Clause "Simplified" License
44 stars 49 forks source link

Add Save Method to NDCube #111

Open DanRyanIrish opened 6 years ago

DanRyanIrish commented 6 years ago

Add a save method to NDCube with the following API: NDCube.save(self, filename, file_type="FITS"): For now it should just support FITS. More file types can be added later. The method should save the information in an NDCube, including data, mask, uncertainty, unit, extra coordinates, WCS, etc., in a standardized way.

Cadair commented 6 years ago

we will have inherited the write method from NDData, I would be interetsed to see what it can do.

DanRyanIrish commented 6 years ago

Good point, I completely forgot about that! But it seems NDCube has not inherited that. It seems the write method is part of NDDataRef, not NDData. See http://docs.astropy.org/en/stable/nddata/ Anyway, NDDataRef.write seems to be for writing to FITS files specifically. It could nonetheless be used in NDCube and hence irispy.

yashrsharma44 commented 5 years ago

Hey @DanRyanIrish ,can NDCube subclass NDIOMixin to support the I/O of fits file? I would love to make a pull request for this.

DanRyanIrish commented 5 years ago

Hi @yashrsharma44. I'm really glad to hear you are interested in addressing this issue and would love to see you write a PR. I'd be very interested to hear more about what NDIOMixin provides. As I understand it just registers read and write methods with the astropy.io.registry rather than actually provides the instructions on how to read or write. Is this correct? If so, can you explain the advantage of inheriting NDIOMixin rather an just writing a write method within ndcube itself?

yashrsharma44 commented 5 years ago

Sorry my bad ;), you were right regarding the interface of NDIOMixin. After a reading through the docs of astropy for NDIOMixin , it was clear that it simply provides the registry of read and write methods. So subclassing NDIOMixin does not provide the implementation( as I wished ;) ). However, we can still subclass NDIOMixin as it creates the registry of the read and write methods. The major task remains, is to code up the write function for the above purpose.

DanRyanIrish commented 5 years ago

OK. Well it's good to have that confirmed. Would you still be interested in working on a PR to do this?

yashrsharma44 commented 5 years ago

Yeah ! I would need some guidance regarding writing up the read write method.

DanRyanIrish commented 5 years ago

OK, great. In that case I can write up an outline of how the write method should work and then we can discuss.

yashrsharma44 commented 5 years ago

Hey @DanRyanIrish , can we discuss the above in some common mailing list?

DanRyanIrish commented 5 years ago

Hi @yashrsharma44. I think it's best to discuss it here so anyone who's interested can see. Then we can update the description at the top of the issue with decisions for easy reference.

DanRyanIrish commented 5 years ago

So an NDCube is comprised of data, uncertainty, mask, meta, wcs, unit, missing_axis, and extra_coords. Let's ignore the extra_coords to start with as they may pose some difficulty.

One structure for saving and NDCube into a FITS file would be:

This I think is pretty intuitive for FITS files. We haven't said where missing_axis or extra_coords can be stored. Perhaps missing_axis can be stored in the hdu0 header with the WCS info in the following way: MISSING0: 1 MISSING1: 0 ... MISSINGN: 0 where the values are 0, 1 indicating False or True.

As for the extra_coords, there is no limit to the number of them, and they can apply to up to N dimensions. So perhaps all hdus after hdu3 be defined as separate extra_coords? E.g.

Issues I see that have no been addressed in the above plan include:

What do you think @yashrsharma44? I think we should also get @Cadair's feedback on this.

yashrsharma44 commented 5 years ago

Sorry for missing out on the discussion! Well, I think that the proposed structure for storing NDCube data is reasonably well designed, so I don't expect any flaws in that. Here are some pointers that I would like to share -

Some queries from my side -

hdu0: header: meta, wcs data: None

What are the different key/value pairs would we store for wcs, besides including some default key/value pairs, and the meta attributes?

WCS axis/axes to which the coord corresponds

Can you elaborate on this ?

Ping @DanRyanIrish If the final writeup is fairly good, then can I start with working on the PR? Though I think, I should wait for @Cadair 's feedback.

DanRyanIrish commented 5 years ago

Hi @yashrsharma44. I'm not sure @Cadair needs to give feedback at this time. Let's push ahead with this plan!

yashrsharma44 commented 5 years ago

Hey @DanRyanIrish , I forgot to ask, what type of FITS data structure are you planning to prefer to store the data for writing NDCube objects?

DanRyanIrish commented 5 years ago

The data, uncertainty, and mask can be primary/image HDUs. @Cadair suggested the extra_coords could be combined into a single Table HDU.

DanRyanIrish commented 3 years ago

@hayesla and @grahamkerr request this functionality in #409. Their conversation has been redirected here. Although note that much of the above discussion was had in the context of ndcube 1.x. In #409 @hayesla said:

After a recent discussion, I had with @grahamkerr, there may be a use case for write functionality of an NDCube (e.g. to a fits file). From what I can tell there doesn't seem to be one at the moment (however maybe I'm missing it?).

The easiest thing I guess would be to save the wcs and meta to a fits header and then save to a fits file with the nd-data array. However this may also not be a good idea, and maybe hard to generalize for sequence/collections.

But something to think about - any thoughts @DanRyanIrish @Cadair

@hayesla, as you see from this thread, we agree with you that a save/write method or function would be very useful. What you suggest would be the easiest thing to do but given the very generalised nature of NDCube there is more to consider. See the above discussions. Since those discussions were had, ndcube 2.0 has been largely implemented, which means we also need to think about how to save the ExtraCoords and GlobalCoords information. I'm sure all these issues are resolvable, but I think we shouldn't try until after ndcube 2.0 is released and the ExtraCoords and GlobalCoords implementations are finalised.

In the meantime, however, if you have more to add about how a write method could be implemented feel free to chime in here. If you would like to work on implementing it once 2.0 has been released, please let us know!

Cadair commented 3 years ago

It's worth remembering that you would only be able to save a NDCube backed by an astropy.wcs.WCS to a FITS file without jumping through a lot of hoops a gWCS is not serialisable to FITS.

DanRyanIrish commented 3 years ago

That's a very good point. So in APE-14 land there is no generalized way to save to FITS? Given the scope of ndcube, that makes me unsure about a save to FITS method. Although perhaps how useful it could be to users might still be a reason to provide it.

DanRyanIrish commented 3 years ago

What about HDF5? If gWCS can be serialized to that and we provide a reader as well, then maybe we don't need to provide a FITS writer.

Cadair commented 3 years ago

gWCS will serialise to asdf, but currently there is no way to save a FITS WCS to asdf (although that could be developed). As I am aware asdf is really the only serialisation option for gWCS at this point.

So in APE-14 land there is no generalized way to save to FITS?

There is no serialisation at all in APE 14, it's very likely that all underlying implementations of APE 14 would have their own serialisation formats.