Repository navigation
Conversation
jkrick
left a comment
There was a problem hiding this comment.
This is a combo of my review along with an AI review. Code runs to completion for me with images etc. These are mostly easy comments to implement, so i will approve now, and I don't need to look at it again..
Will break CI or users:
- Python version error (line 237): f"...{properties["hips_order_min"]}" puts double quotes inside a double-quoted f-string. That only works on Python 3.12 or newer, and CI also tests 3.10 and 3.11. Changing to properties['hips_order_min'] fixes it.
- healpy not installed (line 73): it isn't in tutorial_requirements.txt, so the imports cell will fail in CI. Either add it, or switch to hpgeom, which is already listed (hpgeom.angle_to_pixel(nside, ra, dec, nest=True)).
- URL built like a file path (line 259): os.path.join builds cube_url, which produces backslashes on Windows. Use an f-string or '/'.join.
Correctness and efficiency: - Cube downloaded twice (line 301): [Table.read(cube_url](https://table.read(cube_url/), hdu="SPECTRAL_CHANNELS") downloads the whole cube (about 300 MB) again just to get one small table. Read it from the fits.open call that's already there.
- For the spectrum, consider labeling wavelengths from
spec_channelsinstead ofwavetab, since it's already loaded and describes the cube's own planes. Similarly, consider reading the flux unit from the cube'sBUNITheader rather than hard-codingbunit=u.MJy/[u.sr](https://u.sr/). - Possible crash on the log-scale plot (line 583): it uses norm='log' with vmin set to the 5th percentile, which can be 0 or below for background-subtracted data. If so, plotting raises an error.
- (Minor) The photometry table's
planecolumn is 1-based (np.arange(cube.shape[0]) + 1), whilelayer_id,layer_view, and Firefly'scubeIdxare all 0-based. It doesn't affect the spectrum (wavelengths are attached by row order), but a reader matchingphot_table['plane']againstlayer_viewwould be off by one channel. Consider dropping the+ 1for consistency. - Narrative in section 4. would benefit from more of a description of what a HiPS cube is, ie., what is its shape, what data does it hold, how is this different from other SPHEREx data types, why does this data type exist in the first place, what are good use cases for it?
- Definition of 'hits' map is needed, this is not language I am familiar with.
Housekeeping:
10. Not listed anywhere: the notebook isn't in toc.yml, the SPHEREx gallery (spherex.md), or notebook_metadata.yml, so it won't show up on the site.
11. Cleanup:
- Unused imports: copy, sigma_clipped_stats.
- The !pip install line lists reproject and astroquery, which are never used. It
leaves out healpy, requests and firefly_client.
- Section numbering jumps from 6.1 to 6.3, and 6.3 appears twice.
- There's an empty code cell at the end.
- Typos: "closeset", "Luckliy", "her", "Aromatric".
- explanatory supplement will need to be updated for DR1 location
- runtime: It took me 2min15sec to run.
I make this PR to start the review of the new SPHEREx all-sky HiPS cube notebook.
The notebook runs and everything is in place. @bsipocz might have to check the underlying MD code so that everything fits.
There are some things to be done after the SPHEREx all-sky mosaic is going online:
Link to notebook: https://github.com/Caltech-IPAC/irsa-tutorials/blob/spherex_hips_notebook_faisst/tutorials/spherex/spherex_hipscube.md