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

Draft: cube_fitting.ipynb #71

Merged
merged 13 commits into from
Jan 21, 2020
Merged

Conversation

PatrickOgle
Copy link
Contributor

No description provided.

@PatrickOgle
Copy link
Contributor Author

PatrickOgle commented Nov 12, 2019

Draft notebook for spectral cube fitting. Fit all the spaxels in a Spitzer IRS spectral cube
with continuum + line + PAH feature models, using lmfit. Identify regions by spatial extent and spectral feature ratios. Extract spectra for those regions, then fit more detailed models
to their spectra.

@PatrickOgle PatrickOgle reopened this Nov 12, 2019
@eteq eteq changed the title cube_fitting.ipynb Draft: cube_fitting.ipynb Nov 12, 2019
Copy link
Collaborator

@eteq eteq left a comment

Choose a reason for hiding this comment

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

@PatrickOgle - thanks for this! Mostly looks ok for a draft notebook, but one immediate issue I noticed: you have executed notebook cells. As the guide points out, this is problematic because it leads to notebooks being very large and taking up space in the repo. Can you clear your outputs, commit, and then squash into one commit? (I can show you how to do that if you don't know how.)

Other issue is that the tests did not get triggered. I think this is because one of the settings was wrong on the configuration page that I have access to and have now updated. But we will find out when we push up another commit.

@PatrickOgle
Copy link
Contributor Author

@eteq I did restart the kernel and save before submitting the notebook, and it appeared to clear the output to the cells. Not sure what went wrong here... I may need some help with this.

@eteq
Copy link
Collaborator

eteq commented Nov 26, 2019

I also just realized from looking at it that there's no requirements file (a la https://github.com/spacetelescope/dat_pyinthesky/blob/master/jdat_notebooks/example_notebook/requirements) - can you add one that has the requirements for this notebook? (we may also need to update the jdat_notebooks/environment.yml file to get the tests to run correctly, but I can take care of that if it's needed.)

@hcferguson
Copy link
Contributor

hcferguson commented Jan 7, 2020

@PatrickOgle This is a great draft!

I have lots of comments for future development, but none of them are blockers for merging this PR. Among the comments:

  • Down the road we should be revising this to use the infrastructure we are building, including:

    • specutils
    • spectral-cube
    • astropy models
    • astropy units and quantities
    • astropy fitting
      • model-sets for any linear models can give big speed ups
    • marking spectral lines on the plots (only available in Qt specviz right now I think?)
    • Astropy tables or ASDF for saving the spectral regions.
  • Down the road it would be good to a have a version that incorporates viz tools and some interactive selection. Too early for that now.

  • I wonder if it might help orient the user to have an encapsulated figure from a paper at the start that labels the spectral features?

  • I'd be inclined to keep your functions small and then combine: so fit_and_plot() is just a two line function made up of do_fit() and do_plot() or something.

  • I might suggest reorganizing so that the notebook immediately loads in the data and displays a 2D slice and a 1-d spectrum, to get the user acclimatized to the data before hammering them with a lot of function definitions. Might even illustrate fitting a single line in the 1D spectrum. The rest of the notebook is basically just combining models and extending that workflow across spaxels.

  • Might be interesting to explore saving model results to ASDF files. It may be relatively straightforward to save the results data structure along with the data in that format, which would allow the user to sort of pick up from an already-fit model and extend it. The documentation in lmfit on save_modelresult doesn't really say anything about the format. It looks like it's just a custom ascii format instead of something more portable and human-readable.

  • Probably just your choice of models, but the error bars aren't apparent in the plots in cell 16.

@hcferguson
Copy link
Contributor

In cell 8, it's not obvious to me why you need to initialize to lists:

#Data
data_cube1=[]
error_cube1=[]
wave1=[]
data_cube2=[]
error_cube2=[]
wave2=[]
data_cube1 = cubeSL1[0].data
error_cube1 = errorsSL1[0].data
data_cube2 = cubeSL2[0].data
error_cube2 = errorsSL2[0].data

#Wavelength
xwave1 = cubeSL1[1].data
xwave1 = xwave1.field(0)[0]
xwave2 = cubeSL2[1].data
xwave2 = xwave2.field(0)[0]

@hcferguson
Copy link
Contributor

hcferguson commented Jan 7, 2020

In cell 11, you initialize cube_res outside of your functions, and then use it as an implicit global variable. When I find myself doing this, I usually create a Class with an __init__() method to keep this global info. Your mod_fit and fit_point functions would also be methods of this class. I think it's more transparent in the end, and much safer if people cut & paste.

@hcferguson
Copy link
Contributor

For a future version, it might be nice to have a cell that illustrates putting a contour map (e.g of a spectral feature) on top of an image.

@PatrickOgle
Copy link
Contributor Author

PatrickOgle commented Jan 7, 2020 via email

@hcferguson
Copy link
Contributor

@PatrickOgle Yes, I think this can serve as the science review for this PR.

@PatrickOgle
Copy link
Contributor Author

@eteq @nmearl
Checks are failing on PR #71 , but CI error seems to come from PR #74 (specfit_demo_3.ipynb, not my cube_fitting notebook).

@nmearl
Copy link
Collaborator

nmearl commented Jan 9, 2020

@PatrickOgle The issue seemed to be that you were serializing and deserializing models. This is okay, but the lmfit library requires the dill package to do it consistently. Adding the dill package solves the failing build issue.

@PatrickOgle PatrickOgle requested a review from eteq January 9, 2020 20:36
@PatrickOgle
Copy link
Contributor Author

@eteq Can you resolve your change request and 'pending reviewer'?
This draft notebook is done from my perspective.

@hcferguson hcferguson self-requested a review January 17, 2020 15:33
@eteq
Copy link
Collaborator

eteq commented Jan 21, 2020

Thanks @PatrickOgle @hcferguson, and @nmearl , this is all looking good now!

One lesson learned is we need a smooth way to somehow integrate feedback like @hcferguson's that isn't going in the notebook itself but is I "to do for the next iteration". For now I'm going to create a separate github issue for that, but we may instead want to look into things more tightly coupled to the notebook itself: e.g. "hidden" cells, etc.

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.

4 participants