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

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

Merged
merged 12 commits into from
Jan 31, 2024

Conversation

hiyoneda
Copy link
Contributor

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

  • Changed the axis order of coordsys conversion matrix from [lb, Time/ScAtt, NuLambda] to [Time/ScAtt, lb, NuLamnda]
  • Added docstring to the image deconvolution classes for Sphinx

…inates

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

Also, I modified typos in previous notebooks.
…cAtt, NuLambda] to [Time/ScAtt, lb, NuLamnda]
@hiyoneda
Copy link
Contributor Author

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

@hiyoneda hiyoneda marked this pull request as ready for review January 24, 2024 22:46
@fieldrog
Copy link
Contributor

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
Copy link

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
Copy link
Contributor Author

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.

Copy link
Collaborator

@israelmcmc israelmcmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @hiyoneda! @ckarwin and @fieldrog reviewed it and approved it.

@israelmcmc israelmcmc merged commit 29b08ce into cositools:main Jan 31, 2024
1 check passed
@hiyoneda hiyoneda deleted the image_deconvolution branch March 7, 2024 16:26
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

Successfully merging this pull request may close these issues.

4 participants