PAHFIT / pahfit

Model Decomposition for Near- to Mid-Infrared Spectroscopy of Astronomical Sources
https://pahfit.readthedocs.io/
18 stars 26 forks source link

Initial Spitzer/JWST instrument packs #226

Closed jdtsmith closed 2 years ago

jdtsmith commented 2 years ago

Intended for testing use with PR#222.

codecov-commenter commented 2 years ago

Codecov Report

Merging #226 (168f0aa) into master (7010efa) will increase coverage by 0.06%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
+ Coverage   43.84%   43.91%   +0.06%     
==========================================
  Files          12       12              
  Lines         821      822       +1     
==========================================
+ Hits          360      361       +1     
  Misses        461      461              
Impacted Files Coverage Δ
pahfit/errors.py 100.00% <ø> (ø)
pahfit/features/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7010efa...168f0aa. Read the comment docs.

jdtsmith commented 2 years ago

Torsten Boeker pointed me to some lovely FITS files for NIRSPEC resolution, which I fitted and included.

Some other changes:

Just waiting for similar MIRI values (@karllark!).

karllark commented 2 years ago

Here is the MIRI info Alvaro Labiano just emailed me. He says ignore the "etalon" column (ground-test info).

image

image

jdtsmith commented 2 years ago

Here is the MIRI info Alvaro Labiano just emailed me. He says ignore the "etalon" column (ground-test info).

Awesome, thanks. I'll convert into "plain" a + bx + cx^2 polynomials and stick them in. Do you happen to have the coverage/range for each segment?

karllark commented 2 years ago

See https://jwst-docs.stsci.edu/jwst-mid-infrared-instrument/miri-observing-modes/miri-medium-resolution-spectroscopy

jdtsmith commented 2 years ago

Thanks Karl. I have pushed MIRI data based on these fits, and updated NIRSPEC coverage. I think this is good to go. Could you give it a quick sanity check, look over the naming conventions, etc.?

Testing for a given segment is trivial:

ins='jwst.miri.mrs.ch3.A'
x=np.linspace(*wave_range(ins))
ax.plot(x,resolution(ins,x))
jdtsmith commented 2 years ago

Here's the MRS FWHM plot for reference: jwst_miri_mrs_fwhm.pdf. Pretty big jumps between 3C and 4A.

image

karllark commented 2 years ago

Can you provide a resolution plot as well? That is what is provide in the docs - makes for easier comparison.

karllark commented 2 years ago

It would be great if we had a documentation page for the different instrument packs that describes them and provide the FWHM/resolution plots. These can be generated on the fly in the docs. Lots of examples of this in the karllark/dust_extinction package.

karllark commented 2 years ago

Also, could you fix the codestyle issues that are causing the tests to fail? These were introduced in a previous pull request.

jdtsmith commented 2 years ago

Also, could you fix the codestyle issues that are causing the tests to fail? These were introduced in a previous pull request.

I could, but it would be better if someone else did so to exercise the instrument pack functionality.

jdtsmith commented 2 years ago

Also, could you fix the codestyle issues that are causing the tests to fail? These were introduced in a previous pull request.

Are we using a particular automated linter?

karllark commented 2 years ago

If you run black on the files you update, it will cleanup most of the codestyle issues. That's what I do so that I don't have to learn and remember all the codestyle rules.

karllark commented 2 years ago

We should separate the automated testing issues from getting people to test the code. They serve different purposes.

jdtsmith commented 2 years ago

We should separate the automated testing issues from getting people to test the code. They serve different purposes.

Violent agreement: for sure we need more hands-on testing, and test spectra under data/. Automated tests can follow.

jdtsmith commented 2 years ago

Hey I got a green check! I also figured out how to run flake8 continuously using the same settings from tox.ini, so hopefully I'll be a net entropy reducer now. This is ready to merge. I'm sure we'll need to tweak coefficients as we test against real spectra, but something is better than nothing. Any objections anyone?

jdtsmith commented 2 years ago

Thanks.