search

cositools / cosipy

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

Changed the axis order of coordsys conversion matrix / added doctring to image deconvolution classes #124

Closed hiyoneda closed 5 months ago

hiyoneda commented 5 months ago

This PR should be reviewed after Chris's PR #114 is merged and closed

hiyoneda commented 5 months ago

I need to modify the notebooks for galactic CDS followed by the axis order change in the coordsys conversion matrix. In order not to make things complicated, I merged #120 into this pull request, and closed #120.

The comment originally in #120 is here:

The two notebooks present the image deconvolution with Compton data space in the galactic coordinates (Crab and 511 keV).

cosipy/docs/tutorials/image_deconvolution/511keV/GalacticCDS/511keV-DC2-Galactic-ImageDeconvolution.ipynb cosipy/docs/tutorials/image_deconvolution/Crab/GalacticCDS/Crab-DC2-Galactic-ImageDeconvolution.ipynb

fieldrog commented 5 months ago

Hi @hiyoneda, I'm reviewing your notebook now. I ran through it on a server running Ubuntu 22.04.3, with 515G RAM.

Here is a list of all the issues i encountered with the notebook; most are minor and have to do with the direct data download.

  1. The command to download the 511 keV data is not working as it is currently set up. I believe the filename on wasabi was changed to 511_thin_disk_3months_unbinned_data.fits.gz, and this change needs to be propagated through the nb.

  2. I prefer the way you have the filesystem set up, with path_data, but it does not work (directory structure is incompatible) as it is currently set up in the case that the user downloads the data using the commands in cell 2. I would either download data into a directory structure, or build the rest of the nb to look for the data all in the GalacticCDS directory.

  3. For this to run fully autonomously, need to somewhere check if the psr has been unzipped and if not run: "os.system('gunzip psr_gal_511_DC2.h5.gz')"

  4. I think you need to add a "%matplotlib inline" for the plots to show up, at least in my environment.

As you can see, these are all pretty trivial issues. Otherwise, everything works from scratch on my system. Great job! This is a fun notebook to review :)

KeigoOkuma commented 5 months ago

I ran cosipy/docs/tutorials/image_deconvolution/511keV/GalacticCDS/511keV-DC2-Galactic-ImageDeconvolution.ipynb on my M1 Mac macOS Ventura 13.3.1 (a) with 32GB RAM. It basically worked, but it has a file downloading problem (same as fieldrog).

Regarding the command to download the 511 keV data, I checked Wasabi file_manager as sub-user but I wasn't able to find the file. On the other hand, I was able to get albedo_photons_3months_unbinned_data.fits.gz although I wasn't able to find it in Wasabi too.

I ran 511keV-DC2-Galactic-ImageDeconvolution.ipynb using 511_thin_disk_3months.fits.gz that I obtained earlier.

hiyoneda commented 5 months ago

Thanks @fieldrog @KeigoOkuma!

I modified the notebooks, considering your comments. All of your comments were reflected.

Regarding downloading files, I have checked the file names and modified them to the latest ones. I tested the corresponding cells and confirmed that it works well now.

The number of changed files is large in this PR, but most are due to adding docstring and minor changes on the notebooks.

The thing that was not tested was the notebook with the galactic CDS, and now it has been checked that it works in some different environments. So, I think that this PR is ready to be merged into the main branch.