-
Notifications
You must be signed in to change notification settings - Fork 5
Add spherex_cutouts and spherex_psf #147
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
Conversation
troyraen
commented
Oct 1, 2025
- Transfer spherex_onprem_cutouts.md and spherex_psf.md from ipac-sp-notebooks.
- Rename spherex_onprem_cutouts.md -> spherex_cutouts.md.
- Cleanup.
- Add both notebooks to rendering.
bsipocz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone through the code cells again to try and spot points where we could use more astropy native approaches. Nothing is a blocker, but I feel all of them are good cleanup or teaching points.
| """ | ||
|
|
||
| # Execute the query and return as an astropy Table. | ||
| t1 = time.time() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
| ```{code-cell} ipython3 | ||
| # Define the service endpoint for IRSA's Table Access Protocol (TAP) | ||
| # so that we can query SPHEREx metadata tables. | ||
| service = pyvo.dal.TAPService("https://irsa.ipac.caltech.edu/TAP") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use astroquery.ipac.irsa.Irsa.query_tap? Then there is no need to set the URL
| Note that the values of the rows are being added in place. | ||
|
|
||
| ```{code-cell} ipython3 | ||
| def process_cutout(row, ra, dec, cache): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should instead use a SkyCoord everywhere, after all a SkyCoord is created in here, too, IMO it's just cleaner than dealing with separate ra and dec
|
|
||
| ```{code-cell} ipython3 | ||
| # Write the final MEF | ||
| combined_hdulist.writeto(output_filename, overwrite=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure about the overwrite here? I'm usually rather wary about overwriting data files.
|
|
||
| for ii, img in enumerate(imgs): | ||
| axs[ii].imshow(imgs[ii], norm="log", origin="lower") | ||
| axs[ii].text(0.05, 0.05, r"$\lambda_{\rm center} = %2.4g \,{\rm \mu m}$" % summary_table["central_wavelength"][ii], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way using native mpl plotting of astropy quantities? then there would be no need of manually setting units on the plot, and it ensures it always corresponds to the actual thing you plot
tutorials/spherex/spherex_psf.md
Outdated
| The goal is to obtain the cutout and then extract the PSF corresponding to the coordinates of interest. | ||
|
|
||
| ```{tip} | ||
| To learn more about how to access SPHEREx spectral images and how to download cutouts, we refer to the [SPHEREx Intro Tutorial](https://caltech-ipac.github.io/irsa-tutorials/tutorials/spherex/spherex_intro.html1) and the [SPHEREx Cutout Tutorial](https://caltech-ipac.github.io/irsa-tutorials/1). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should do better referencing now that everything is in the same repo. Please add labels to the other tutorials and cross ref those; we should even have nice pop-ups.
| ra = 305.59875000000005 * u.degree | ||
| dec = 41.14888888888889 * u.degree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are way too many digits here. But more importantly, I think we should switch to use a SkyCoord in these tutorials
| ```{code-cell} ipython3 | ||
| # Define the service endpoint for IRSA's Table Access Protocol (TAP) | ||
| # so that we can query SPHEREx metadata tables. | ||
| service = pyvo.dal.TAPService("https://irsa.ipac.caltech.edu/TAP") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment, maybe use astroquery's query_tap
|
once you have a passing github buildhtml job, that label can be removed. |
|
Oh I'm sorry, I thought all content reviews and changes were done in https://github.com/IPAC-SW/ipac-sp-notebooks/pull/131 and https://github.com/IPAC-SW/ipac-sp-notebooks/pull/113. Maybe we need a better system for indicating when those types of reviews are done so that when a notebook is transferred to this repo we're not trying to mix in content changes with the same PR. |
|
Now looking back at those PRs, a few of these came up there already, but now as I had another look at the rendering I run into them again. None of them are critical, so you could bump them in a follow-up issue. |
| A good value for the maximum number of workers is between 7 and 12 for a machine with 8 cores. | ||
| ``` | ||
|
|
||
| ```{tip} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this box is repeated from the previous section, consider to just keep one of them?
1f795fc to
7a4d7fe
Compare
|
@bsipocz this is passing now. Need an approving review in order to merge. |
bsipocz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!