cositools / cosipy

The COSI high-level data analysis tools
Apache License 2.0
3 stars 16 forks source link

Updated image deconvolution module, following the change in the orientation module #88

Closed hiyoneda closed 7 months ago

hiyoneda commented 9 months ago

Following the new orientation module, I updated the image deconvolution module.

To avoid saving lots of intermediate files, I also modified SpacecraftFile. Though the changes are very small (when "quiet == True," I turned off the lines related to the file saving.), I would like to ask @Yong2Sheng to have a look at it, just in case.

Finally, I updated the notebook renaming it as "GRB-miniDC2-image-deconvolution.ipynb". I have also deleted some unused/out-dated parameter files.

eneights commented 8 months ago

This looks good @hiyoneda! The notebook ran for me with one small change: In data_loader.py, from cosipy.SpacecraftFile import SpacecraftFile needs to be from cosipy.spacecraftfile import SpacecraftFile.

I get an error (AttributeError: 'Axis' object has no attribute 'coordsys') when running dataloader.load_coordsys_conv_matrix_from_filepath("coordsys_conv_matrix.hdf5"), but this doesn't seem to affect the rest of the notebook.

Overall, the notebook could be cleaned up a bit. It was unclear to me what many of the cells were doing and whether they were all necessary, so more comments would be useful. For example, it says in the SC orientation section that only the first two cells are required, and the notebook works for me without them, so it's not clear to me why the rest of those cells are there.

The last cell loads the results from saved files, but how are these files created? I don't see this anywhere in the notebook.

A few details: It was decided to only use the test_data folder for files needed to run the unit tests (see the discussion on PR #93), and to upload larger files to wasabi. Could you create a folder (docs/tutorials/image_deconvolution) and add the notebook, along with imagedeconvolution_parfile_GRB_miniDC2.yml and orientation file there? This will also keep the tutorials directory more organized.

GRB-end-to-end.ipynb is a good example of the full analysis of a GRB, but docs/tutorials/spectral_fits/ has more up to date versions of the spectral fitting notebooks.

The Google drive folders containing the response were rearranged, so the link in https://github.com/israelmcmc/cosipy/tree/master_grb/docs/tutorials/end-to-end/data does not lead directly to the response file.

hiyoneda commented 8 months ago

Thanks, @eneights, for reviewing the code. I created a directory (docs/tutorials/image_deconvolution) and moved the notebook and related parameter files there. I also modified the notebook by adding more information. I would be happy if you could run through it again!

eneights commented 8 months ago

Thank you @hiyoneda! Just a few more minor changes before merging:

Currently, to run dataloader.calc_coordsys_conv_matrix(), the detector response needs to be in the same directory as the notebook. Could you change this to use the path defined by path_data?

Please add a cell at the beginning so that users can download the necessary files directly from wasabi. You can copy the code from the first cell of SpectralFit.ipynb.

Can you move all of the import statements to a cell at the beginning of the notebook?

Please make the data path generic instead of specific to your computer (e.g. 'path/to/data/').

Could you collapse the output from all of the cells that have long outputs? In particular, the one that runs the image deconvolution (after "4-5. Start the image deconvolution"), as well as the one right after that, but there might be more.

Please add a brief description of what each cell is doing from section 4-5 to the end.

hiyoneda commented 7 months ago

Thanks again. I think that I have modified the notebook following your suggestion.

eneights commented 7 months ago

@hiyoneda Could you just collapse the output of the cells where it prints the image deconvolution results (second cell of 4-5), where it plots the images in each energy band (second cell of “The reconstructed images”), and where it plots the differences between images (second and third cells of “Delta image”)? After that, I think it's ready to merge!

ckierans commented 7 months ago

I ran through this with no issues and I think the GRB-miniDC2-image-deconvolution notebook is ready to merge. It would be good if @eneights confirms, since she's been reviewing the code in more detail.