PAHFIT / pahfit

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

Extending PAHFIT to include AKARI #116

Closed ThomasSYLai closed 2 years ago

ThomasSYLai commented 3 years ago

This is a relatively large PR that includes the capability of fitting the combined AKARI+Spitzer spectrum, with a wavelength coverage of 2.5-38 µm. The objective of this PR is to work on #15 that will set the milestone of 2.1.

Recaps on this PR:

At this point, I didn't separate AKARI and make it a standalone scipack. May need more discussions and effort to build the pack generator (in relation to the discussion in #88). This can be worked along the way. This PR only focuses on the capability of fitting the AKARI+Spitzer spectrum.

The parameters of features that lie beyond 5 µm in AKARI+Spitzer scipack was set following the Spitzer scipack (scipack_ExGal_SpitzerIRSSLLL.ipac), except (i) PAH feature at 5.27 is split into two features, i.e, PAH 5.24 and PAH 5.33 (ii) the initial value of amp of S07_att is set to 0.1 rather than 1 in scipack_ExGal_SpitzerIRSSLLL.ipac.

Here is a quick look at the result of the 1C spectrum adopted from Lai et al. (2020). image

To run the fit and output the plot, simply type: run_pahfit 1C_snr_gt20.ecsv scipack_ExGal_AkariIRC+SpitzerIRSSLLL.ipac --estimate_start plot_pahfit 1C_snr_gt20.ecsv 1C_snr_gt20_output.ipac

Closes #15 Closes #75

codecov-commenter commented 3 years ago

Codecov Report

Merging #116 (41a4bfc) into master (3ea53e1) will decrease coverage by 0.25%. The diff coverage is 48.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
- Coverage   51.74%   51.48%   -0.26%     
==========================================
  Files           8        8              
  Lines         516      538      +22     
==========================================
+ Hits          267      277      +10     
- Misses        249      261      +12     
Impacted Files Coverage Δ
pahfit/base.py 54.16% <40.00%> (-0.96%) :arrow_down:
pahfit/component_models.py 89.58% <58.33%> (-10.42%) :arrow_down:

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 3ea53e1...41a4bfc. Read the comment docs.

karllark commented 3 years ago

This is awesome! I'll give it a try and review hopefully this week.

One quick request, can you provide a file with a spectrum that can be used to test this PR? Ideally it would be great if we could include such a spectrum in the repository just like the M101 spectrum.

ThomasSYLai commented 3 years ago

@karllark the spectrum that I used here is already in the data directory, namely 1C_snr_gt20.ecsv. Do you think we can keep the file as .ecsv or need to change it accordingly to .ipac file. One good thing to keep it as .ecsv is that it demonstrates the capability of PAHFIT to read the .ecsv file. But I'm fine with either way.

I have also edited the comment above on how to run the result.

karllark commented 3 years ago

@karllark the spectrum that I used here is already in the data directory, namely 1C_snr_gt20.ecsv. Do you think we can keep the file as .ecsv or need to change it accordingly to .ipac file. One good thing to keep it as .ecsv is that it demonstrates the capability of PAHFIT to read the .ecsv file. But I'm fine with either way.

So it is. It has been there so long I forgot about it. Fine keeping it as an ecsv file. I agree - helps illustrated pahfit can handle multiple formats.

I have also edited the comment above on how to run the result.

Great! Thanks.

ThomasSYLai commented 3 years ago

@karllark One test didn't pass --- link-test. It seems that I am encountering this linking issue again. It's trying to link to tir at UToledo. Anything I can do to fix it?

karllark commented 3 years ago

The link test failed for a known issue with an astropy URL. Will be fixed in PR#113.

karllark commented 3 years ago

Will review and likely merge soon. Need to tag the current version as v2.0 first before merging this PR what is providing most (all) of v2.1. A bit of scheduling needed to get things to be organized as we planned.

karllark commented 3 years ago

Waiting to merge this into the master till after the master is tagged with v2.0. One documentation PR (IDL vs python fitting comparison) planned before v2.0 tagging.

karllark commented 2 years ago

There is a conflict now. @ThomasSYLai can you rebase this PR to the current master? If you haven't done this before, I'd be happy to walk you through this.

ThomasSYLai commented 2 years ago

More rebasing conflicts to fix.

Conflicts fixed. Code ready to merge. A quick look at the latest fitted results. Will work on Docs for v2.1. image

jdtsmith commented 2 years ago

Final question occurred to me @ThomasSYLai — did you confirm that you can run this new v2.1 branch with either of the Science Packs (with/without Akari)?

karllark commented 2 years ago

Final question occurred to me @ThomasSYLai — did you confirm that you can run this new v2.1 branch with either of the Science Packs (with/without Akari)?

The tests are without AKARI, so at least that case works.

ThomasSYLai commented 2 years ago

@jdtsmith The v2.1 branch should work in both the with and without akari cases.