Repository navigation
Conversation
jkrick
left a comment
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.
|
Thank you, @jkrick. Implemented all of these. |
|
New push, I see this as the final before the HiPS cubes are life.
|
jaladh-singhal
left a comment
jaladh-singhal
left a comment
There was a problem hiding this comment.
@afaisst firefly part looks good, it can use some minor improvements.
| Now we add a new Firefly instance to plot the spectrum, we call it `fc_spec`. | ||
|
|
||
| ```{code-cell} ipython3 | ||
| fc_spec = FireflyClient.make_client(url="https://irsa.ipac.caltech.edu/irsaviewer") | ||
| ``` |
There was a problem hiding this comment.
You can reuse fc. You don't need to open another connection to Firefly viewer, consider removing this.
If you wish to clean state of your old viewer containing HiPS, before displaying spectrum, you can do fc.reinit_viewer() here. But I personally think showing HiPS and spectrum together is nice since Firefly composes different image/table/chart views anyways.
There was a problem hiding this comment.
but I still have the other instance open, I want to have two Firefly windows...
There was a problem hiding this comment.
Ah ok, then up to you - fine to leave it.
| ``` | ||
|
|
||
| ```{code-cell} ipython3 | ||
| fc_spec.show_table(file_input=phot_table_firefly, |
There was a problem hiding this comment.
You can directly use the fc from above. This will show your spectrum in the same viewer that was showing HiPS.
| fc_spec.show_table(file_input=phot_table_firefly, | |
| fc.show_table(file_input=phot_table_firefly, |
Updated instructions for loading emission lines to reflect changes in button naming in the viewer.
|
Pushed a new version. The links are not ready, all "irsa" now (and not "irsadev"). I also had to change the hips_url to two URLs, one for FireFly and one for reading the FITS cubes directly. |
troyraen
left a comment
troyraen
left a comment
There was a problem hiding this comment.
I took a quick look and flagged urls that need attention.
|
|
||
| The community will also mine SPHEREx data and combine it with synergistic data sets to address a variety of additional topics in astrophysics. | ||
|
|
||
| More information is available in the [SPHEREx Explanatory Supplement](https://irsa.ipac.caltech.edu/data/SPHEREx/docs/SPHEREx_Expsupp_QR.pdf). |
There was a problem hiding this comment.
Update this url to https://irsa.ipac.caltech.edu/data/SPHEREx/docs/SPHEREx_Expsupp.pdf. That will be a new Explanatory Supplement covering all data releases. (It's currently live on irsatest, in case you want to look at it.)
|
|
||
| ```{code-cell} ipython3 | ||
| hips_url = "https://irsa.ipac.caltech.edu/data/hips/SPHEREx/spherex_allsky_cube" | ||
| hips_data_url = "https://irsa.ipac.caltech.edu/ibe/data/spherex/dr1/hips5_cube/hips_20261026" |
There was a problem hiding this comment.
I see this hips_data_url on irsadev now, but I know we recently received more HiPS data and what's on IBE is still in flux. We should check in a few days and determine whether this is still the right url. Even if this particular one is still on dev, there may be other data with a slightly different url that's more appropriate to use here. Input from some combo of Anastasia, Vandana, and Andreas may be needed.
There was a problem hiding this comment.
I asked about this in IRSA leads meeting this morning and both of these urls should be fine.
The HiPS cubes are now available in SIA, which we did not expect to happen before Nov 12. That means we shouldn't need hips_data_url at all because we shouldn't need to manually construct the url. We can revisit that after Nov 12. No changes needed right now.
|
|
||
| ```{note} | ||
| If you look at the Firefly visualization, you will see some striping happening. This striping is an artifact originating from the mosaic creation close to bright emission lines such as Pa-$\alpha$ (1.88$\mu$m), PAH (3.3$\mu$m), or Br-$\alpha$ (4.05$\mu$m), which are strongly emitted by dust and hydrogen gas in our Milky Way. While these artifacts affect the mosaic, they do not affect the photometry itself if extracted from the [IRSA SPHEREx spectrophotometry tool](https://irsa.ipac.caltech.edu/applications/spherex/tool-spectrophotometry). | ||
| For more information, see the "Mosaics" Section in the [SPHEREx Explanatory Supplement](https://irsa.ipac.caltech.edu/data/SPHEREx/docs/SPHEREx_Expsupp_QR.pdf). |
There was a problem hiding this comment.
Same as above, update the url.
| * The `NHITS` extension shows the number of contributing input pixels per mosaic pixel. | ||
| * The `FLAGS` extension includes bitwise OR of flags propagated from contributing input pixels. | ||
|
|
||
| For more information on these layer and the FITS cube construction, we refer to the "Mosaics" Section of the [SPHEREx Explanatory Supplement](https://irsa.ipac.caltech.edu/data/SPHEREx/docs/SPHEREx_Expsupp_QR.pdf). |
Closes #393
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