SPHEREx Mosaic Tool#311
Conversation
There was a problem hiding this comment.
We plan to post the data file on the IRSA website so that we don't have to store it in the repo (IRSA-7723). Assuming that works out, I think this .md notebook file can be moved up a level so it's directly under tutorials/spherex/ rather than in a dedicated spherex_mosaic directory.
|
@jkrick will you review this? |
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
There are some formatting issues that result in this failing all the tests. And I have also done a first round run through, eyes mostly on design and API considerations as well as minor cleanups.
We have also talked about hiding some cells in dropdowns. For now you have have code cells that uses the source_hidden metadata as we already use it in other notebooks. While it doesn't hide the cell in the HTML rendering, I'm working on finding a solution that does and it would be easier to fix these all at once if the same metadata is being used everywhere.
```{code-cell} ipython3
---
jupyter:
source_hidden: true
---
| wave_tab | ||
| ``` | ||
|
|
||
| <!-- #region --> |
There was a problem hiding this comment.
we need to double check how this region thing looks in rendering
There was a problem hiding this comment.
not quite sure what that is...? The output is a table in that cell.
| <!-- #endregion --> | ||
|
|
||
| ```python | ||
| def get_aperture_photometry(cube , r_aperture_px , r_inner_px , r_outer_px, MAKEPLOT): |
There was a problem hiding this comment.
preferably we should use keyword arguments for API design, so it won't end up in lines of somewhat cryptic numbers like in old IRAF/IDL days.
There was a problem hiding this comment.
tried to fix this. Check...
There was a problem hiding this comment.
please squash out these changes from the commit
bsipocz
added
the
content: spherex
|
@afaisst can you please send me the fits file some other way (google drive?) so I can test/review while we wait for whatever else needs to be worked out? |
|
@jkrick you can get the fits file from the jira ticket I linked above. |
That is great, thank you. If we know we won't be changing the file ever then we can get away with adding it into the repo (it's ~83MB), but if there will be iterations with it then it can very quickly blow things up. |
jkrick
left a comment
jkrick
left a comment
There was a problem hiding this comment.
Overall the tutorial is a great idea. I have a few major comments below to simplify things (use WCS.celestial and plane identification by wavelength not plane number), and other smaller comments.
|
|
||
|
|
||
| ## Plotting stuff | ||
| def load_plotting_defaults(): |
There was a problem hiding this comment.
This can be hidden, if/when we figure out how to hide cells.
There was a problem hiding this comment.
yeap, then just add that heading to any of the cells you want to dropdown/hide, so we don't need to do a content review again once the technical part is figured out.
|
|
||
| In addition, we also adjust the `BUNIT` keyword in the header to reflect the changes. | ||
|
|
||
| ```{warning} |
There was a problem hiding this comment.
this whole messiness can be simplified by using WCS.celestial (thanks Claude for figuring that out). @afaisst I have a commit that makes this change, let me know if you want me to push it here.
|
ok, I resolved most comments. There are a few left: @bsipocz : can you check the syntax in the functions? Doc strings and keyword arguments? Also, I separated out the cell with the plotting stuff in it so we can make this dropdown/hide cell. |
| def measure_aperture_photometry_cube(cube, | ||
| r_aperture_px = 20, | ||
| r_inner_px = 60, | ||
| r_outer_px = 100, | ||
| MAKEPLOT = True | ||
| ): |
There was a problem hiding this comment.
It's something like this instead:
| def measure_aperture_photometry_cube(cube, | |
| r_aperture_px = 20, | |
| r_inner_px = 60, | |
| r_outer_px = 100, | |
| MAKEPLOT = True | |
| ): | |
| def measure_aperture_photometry_cube(cube, *, r_aperture_px=20, r_inner_px=60, r_outer_px=100, makeplot=True): |
note that I also changed the caps on makeplot here, but not in the code below, it should not be all caps.
| # !pip install astropy matplotlib numpy photutils reproject astroquery | ||
| ``` | ||
|
|
||
| ```python |
There was a problem hiding this comment.
| ```python | |
| ```{code-cell} ipython3 |
|
Tried to redo the conversion: |
4 participants
First version of the SPHEREx mosaic tool tutorial notebook.
There is also a FITS file attached, which is the SPHEREx mosaic created by the IRSA SPHEREX mosaic tool GUI. The notebook needs it to run. It's currently blocked from pushing that file because I guess it's on the git-ignore list (because FITS file...)