Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ifu continuum #151

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Ifu continuum #151

wants to merge 12 commits into from

Conversation

orifox
Copy link
Contributor

@orifox orifox commented Aug 31, 2021

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.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@pllim
Copy link
Contributor

pllim commented Sep 1, 2021

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.

Screenshot 2021-09-01 092529

@PatrickOgle
Copy link
Contributor

PatrickOgle commented Sep 13, 2021

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-6
u.m, 1.62E-6*u.m]
spec1d_t = spectral_slab(spec1d, trim[0], trim[1])
cube_trimmed = spec1d_t.flux.value

#Adjust wcs to trimmed cube because spectral_slab doesn't
new_wcs = spec1d_t.wcs
new_wcs.wcs.crval[0] = trim[0].value #Change wave crval to match trim region
print(new_wcs)

  1. Load the trimmed Spectrum1D cube into cubeviz.

  2. It is not explained in text that Subset4 and Subset5 spectral regions need to be selected in Cubeviz before continuing.

  3. I am running into errors in the cell that retrieves the spectral regions, and can proceed no further:


AttributeError Traceback (most recent call last)
/var/folders/cw/tytqv5y91qd3j4_stwnc03c80001ww/T/ipykernel_30567/1159788268.py in
1 #Extract the spectral regions defined in the spectral viewer
2 from specutils.spectra import SpectralRegion
----> 3 regions = cubeviz.specviz.get_spectral_regions()
4 regions
5

AttributeError: 'CubeViz' object has no attribute 'specviz'

@PatrickOgle
Copy link
Contributor

Science Review (Part 2):
Continue review, using latest development version of jdaviz.

  1. 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.

  2. 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.

  3. 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.

  4. 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.

  5. 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...

  6. The final plot needs a caption to explain what is going on and what the final results are.

Great work, Tracy and Ori!

Copy link
Contributor

@PatrickOgle PatrickOgle left a comment

Choose a reason for hiding this comment

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

Science review: looks great. I left a number of detailed comments in the PR that are optional suggestions.

@PatrickOgle
Copy link
Contributor

Here is an example loading the trimmed Spectrum1D into Cubeviz
ngc4151_trimmed_spec1d

@ojustino
Copy link
Collaborator

ojustino commented Sep 14, 2021

Hi @orifox, please find the technical review below.

Before you begin

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 [email protected]: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.


Instructions

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:

image

(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:

image

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.

The three-point review (*action required*)

  1. ✅/⚠️ Notebook execution: I was able to run the notebook from top to bottom, with some caveats.

    • Like @PatrickOgle, I had to install the latest version of jdaviz to get things working.
    • There are a couple of attempts to get subsets from 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.
    • The videos are a mixed bag for me. In Firefox, all of them loaded except the last one, and then I couldn't load any more video cells. This even persisted when I restarted the notebook. There's a repeated warning to use IPython.display.IFrame instead, but that doesn't help, either.
  2. ⚠️ Code style: There are several PEP8 violations.

    • Please follow the advice in the "Instructions" section above to fix them.
  3. ⚠️ Notebook style:

    • As @PatrickOgle said, it would be nice to see more Markdown text, both for the sake of readability and to take better advantage of the notebook file format. A quick start is to migrate some of the longer block comments into their own Markdown cells.
    • There's a lot of commented code, which is normally not acceptable in a final product. Is all of it necessary? For those that survive a critical edit, you could consider changing the cell that contains them from a code cell into "raw" by selecting the cell and then pressing the "R" key.

@ojustino ojustino force-pushed the ifu_continuum branch 2 times, most recently from f85fb59 to 2109958 Compare September 15, 2021 18:16
@ojustino ojustino mentioned this pull request Sep 15, 2021
Copy link
Collaborator

@ojustino ojustino left a comment

Choose a reason for hiding this comment

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

I'm giving a pass on the technical reviews after today's edits. I will wait to see how the tests look before merging.

Copy link
Contributor

@PatrickOgle PatrickOgle left a comment

Choose a reason for hiding this comment

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

I was able to successfully run through the notebook. There was one Cubeviz GUI error when fitting the continuum subtracted cube that didn't appear to affect the result. The science workflow is clear, but still could benefit from trimming the cube to remove extraneous data and improve the presentation.

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.

8 participants