Skip to content

SPHEREx all-sky cube mosaic notebook - #397

Open
afaisst wants to merge 1 commit into
mainfrom
spherex_hips_notebook_faisst
Open

afaisst wants to merge 1 commit into
mainfrom
spherex_hips_notebook_faisst

Conversation

@afaisst

@afaisst afaisst commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

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

@afaisst afaisst self-assigned this Oct 23, 2026
@troyraen
troyraen requested review from jkrick and removed request for troyraen October 23, 2026 19:47

@jkrick jkrick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. 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)).
  3. 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:
  4. 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.
  5. For the spectrum, consider labeling wavelengths from spec_channels instead of wavetab, since it's already loaded and describes the cube's own planes. Similarly, consider reading the flux unit from the cube's BUNIT header rather than hard-coding bunit=u.MJy/[u.sr](https://u.sr/).
  6. 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.
  7. (Minor) The photometry table's plane column is 1-based (np.arange(cube.shape[0]) + 1), while layer_id, layer_view, and Firefly's cubeIdx are all 0-based. It doesn't affect the spectrum (wavelengths are attached by row order), but a reader matching phot_table['plane'] against layer_view would be off by one channel. Consider dropping the + 1 for consistency.
  8. 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?
  9. 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.

This branch has not been deployed

No deployments
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.

2 participants