Open orifox opened 3 years ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
Note to reviewer/maintainer: There are large movie files in the commits that have since been deleted and need to be squashed out of the history before merge.
Either use git rebase -i HEAD~n
to squash them via CLI and force push or use the "Squash and Merge" button.
Science Review (Part 1):
The science workflow is generally sound, but could be tightened up. I would like to make the following suggestions:
1) Read the datacube directly into the notebook using Spectrum1D.read. This would avoid having to set up a separate wavelength array. However, I can't find a format specification that works with this dataset. May need a new format specification for Spectrum1D.read.
2) Trim the Spectrum1D cube using specutils.manipulation.spectral_slab, to get rid of the bad wavelengths at either end of the spectrum and avoid the problems with autoscaling. Even better, trim the Spectrum1D cube to contain only the lines and continuum region of interest. This will make the model fitting step more straightforward. It would be nice to have a Jdaviz Plugin that trims spectra.
This code accomplishes that:
!pip install specutils --upgrade import astropy.wcs as fitswcs from specutils.manipulation import spectral_slab
my_wcs = fitswcs.WCS(header_cube) spec1d = Spectrum1D(flux = cubeu.Jy, wcs=my_wcs) trim=[1.57E-6u.m, 1.62E-6*u.m] spec1d_t = spectral_slab(spec1d, trim[0], trim[1]) cube_trimmed = spec1d_t.flux.value
new_wcs = spec1d_t.wcs new_wcs.wcs.crval[0] = trim[0].value #Change wave crval to match trim region print(new_wcs)
3) Load the trimmed Spectrum1D cube into cubeviz.
4) It is not explained in text that Subset4 and Subset5 spectral regions need to be selected in Cubeviz before continuing.
AttributeError Traceback (most recent call last)
/var/folders/cw/tytqv5y91qd3j4_stwnc03c80001ww/T/ipykernel_30567/1159788268.py in
AttributeError: 'CubeViz' object has no attribute 'specviz'
Science Review (Part 2): Continue review, using latest development version of jdaviz.
6) The text description of model fitting should be expanded. There is a list of steps to do in the GUI, but they are not explained fully in text. The video demonstrations of how to do this are actually quite nice for this purpose, but should be supplemented by more text. In particular, it is somewhat confusing to distinguish between the Data selection and the Spectral Region selection. I spent some minutes looking for Subset 5 under Data, even though I should know better. Of course this is all explained in the videos, but I don't want to have to keep going back to the videos for reference.
7) In the subtract continuum section, the original cube is read in again. This shouldn't be necessary since it was already read in one of the first few cells.
8) Visualizing the Continuum-Subtracted data. It is impressive to look at the slices of the continuum - subtracted cube, particularly slices 1060:1090 to visualize the velocity field of the emission line.
9) Again, the text description about fitting the 3-component Gaussian model should be expanded. There is no text description of first fitting with fixed stdev and mean, then re-fitting with free parameters. The videos are again very helpful.
10) It is painful to enter the starting parameters for all of the Gaussian model components. I wish there was a way to load model components and parameters into the plugin via the notebook...
11) The final plot needs a caption to explain what is going on and what the final results are.
Great work, Tracy and Ori!
Here is an example loading the trimmed Spectrum1D into Cubeviz
Hi @orifox, please find the technical review below.
The technical review helps ensure that contributed notebooks a) run from top to bottom, b) follow the PEP8 standards for Python code readability, and c) conform to the Institute's style guide for Jupyter Notebooks.
I've pushed the review as a new commit in this pull request. To view and edit the commit locally, follow these steps:
git checkout ifu_continuum
git fetch YOUR-REMOTE-FORK
git rebase YOUR-REMOTE-FORK/ifu_continuum
_(YOUR-REMOTE-FORK
is your fork's online copy. It's often origin
, but if you don't know your name for it, run git remote -v
and choose the one whose path ends with git@github.com:YOURUSERNAME/dat_pyinthesky.git
.)_
From here you can work on your branch as normal. If you have trouble with this step, please let me know before continuing.
After updating your local copy of this branch, please open your notebook and address any warnings or errors you find.
If you see cells with output like this, it means some of your code doesn't follow the PEP8 standards of code readability:
(In the example above, INFO - 3:3: E111
means that the text entered on line 3 at index 3 caused the warning "E111". The violation is briefly described at the end of the message.)
You can test that your edits satisfy the standard by installing flake8
on the command line with:
pip install flake8 pycodestyle_magic
Then, restart the notebook and run the following cells:
After that, edit and re-run cells with warnings until you've fixed all of them. Please remember to delete the cells shown in the above image before pushing your changes back to this pull request.
If you have questions or feedback on specific cells, click the earlier message in this thread from the "review-notebook-app" bot. There, you can comment on specific cells and view what's changed in the new commit. I may also write comments there, though all comments made on that page will also be reflected in this pull request's conversational thread.
✅/⚠️ Notebook execution: I was able to run the notebook from top to bottom, with some caveats.
jdaviz
to get things working.region
objects that fail; these are the cells with KeyError
output. It looks safest to stick to cubeviz.app.get_data_from_viewer()
when extracting subsets.IPython.display.IFrame
instead, but that doesn't help, either.⚠️ Code style: There are several PEP8 violations.
⚠️ Notebook style:
Updated Tracy's notebook on IFU Continuum fitting to be what I consider Advanced: 1) Got rid of all Dev Notes 2) Moved almost all the analysis into Jdaviz 3) Streamlined the workflow
I'd like Patrick, Cami, Larry, and Erik to check it out, especially since this is one of the more public displays of using Jdaviz. Also consider if you like the way I embedded videos. It's not consistent with what we discussed before, but you'll see that describing these actions via text was nearly impossible and I really see a lot of benefit to embedding rather than taking users to different sites. But very open minded to your reviews.