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

Major update for initializing via a file, ingest of spectra, generalizing constraints, scripts, etc. #51

Merged
merged 15 commits into from
Mar 6, 2019

Conversation

karllark
Copy link
Contributor

@karllark karllark commented Feb 20, 2019

Major changes to enable a fit to be done starting with a file instead of the code setup of parameters, etc. Building off the work already done by @alexmaragko on the write/read functions for an astropy Table format file. Later the more flexible YAML file format will be added. But for now, just getting things working for v2.0.

Should address (or partially address) issues #46, #31, #28, #27, and #1.

  1. Generalized the parameters for all the components. Now all can have limits and be fixed. This includes the temperatures for the blackbody components. This means that the user can specify the starting values for all variables, fix any variable, and provide constraints (or not) for any variable. Significant code changes were required to make this possible for all variables.

  2. Plotting script created to plot saved results.

  3. Code that can be run (e.g., run_pahfit or plot_pahfit) moved to be in the scripts directory. These scripts are installed when installing pahfit allowing these scripts to be run from any directory w/o giving the full path to the script.

  4. Renamed the "base" of PAHFIT from PAHFITBase.py to base.py. This makes imports easier. E.g., 'from pahfit.base import PAHFITBase' instead of 'from pahfit.PAHFITBase import PAHFITBase'.

  5. First real continuous integration test code. Tests if the "classic" pack file on disk (in ipac format) matches a static version encoded in python code. Ensures that the classic data file does not change. Keeping the python code to allow this test as well as providing an example of generating pack info from code instead of via a file (interesting for advanced users). This test naturally emerged from checking the save/read code and revealed bugs. Instead of making it a one-off test, I went ahead and made a continuous integration test that will run with every pull request ensuring no such bugs reemerge!

  6. Starting documentation for running pahfit and plotting the results via the run_pahfit and plot_pahfit scripts. Docs updated for this pull request can be seen at http://www.stsci.edu/~kgordon/pahfit/ (after PR merged, these docs will appear automatically at the readthedocs link).

  7. Added support and command line option for reading in observed spectra. Includes support for units
    through astropy.units. This requires that the input observed spectrum file has units defined.

@karllark karllark added this to the v2.0 milestone Feb 20, 2019
@karllark karllark added enhancement testing documentation science packs Features by science area (e.g., exgal, agb, etc) labels Feb 20, 2019
@karllark karllark self-assigned this Feb 20, 2019
@karllark
Copy link
Contributor Author

This pull request introduces 1 alert when merging ed7bc13 into 8c21419 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

Comment posted by LGTM.com

@karllark karllark removed this from the v2.0 milestone Feb 20, 2019
@karllark
Copy link
Contributor Author

This pull request introduces 1 alert when merging 8838ea3 into 8c21419 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

Comment posted by LGTM.com

@coveralls
Copy link

coveralls commented Feb 20, 2019

Coverage Status

Coverage increased (+54.7%) to 67.982% when pulling b8fffe4 on karllark:update_init into 8c21419 on PAHFIT:master.

@karllark
Copy link
Contributor Author

This pull request introduces 1 alert when merging a017f7c into 8c21419 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

Comment posted by LGTM.com

@karllark karllark changed the title WIP: enable starting model with file Major update for initializing via a file, ingest of spectra, generalizing constraints, scripts, etc. Feb 25, 2019
@karllark
Copy link
Contributor Author

Ready for review. Lots of changes!

@alexmaragko , @jdtsmith , @els1 , @ThomasSYLai : you can see the documentation that will be on readthedocs once this pull request is merged at http://www.stsci.edu/~kgordon/pahfit/.

@els1
Copy link
Contributor

els1 commented Feb 25, 2019 via email

@karllark
Copy link
Contributor Author

Great. Thanks @els1!

docs/fit_spectrum.rst Outdated Show resolved Hide resolved
docs/fit_spectrum.rst Outdated Show resolved Hide resolved
docs/index.rst Show resolved Hide resolved
docs/plot_result.rst Outdated Show resolved Hide resolved
docs/plot_result.rst Outdated Show resolved Hide resolved
@jdtsmith
Copy link
Contributor

jdtsmith commented Mar 1, 2019

I wonder if it would be valuable to have one default input unit, for example mJy, in case units are not provided. Is the plan to convert unit implicitly to an internal f_nu unit system?

pahfit/base.py Outdated Show resolved Hide resolved
pahfit/base.py Outdated Show resolved Hide resolved
@alexmaragko
Copy link
Contributor

I wonder if it would be valuable to have one default input unit, for example mJy, in case units are not provided. Is the plan to convert unit implicitly to an internal f_nu unit system?

Yes the units are converted currently in Jy and microns in run_pahfit.py lines 66-71. It's not clear to me thought how it gets the current units from the ipac file (which currently has units of Jy and microns). Otherwise if no units are assigned before converting it would raise an error.

pahfit/base.py Outdated Show resolved Hide resolved
Copy link
Contributor

@alexmaragko alexmaragko left a comment

Choose a reason for hiding this comment

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

Looks good overall. Looking forward to trying out the new version!

Copy link
Contributor

@els1 els1 left a comment

Choose a reason for hiding this comment

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

Looking good.

pahfit/base.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jdtsmith jdtsmith left a comment

Choose a reason for hiding this comment

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

Looks good! 2.0 getting ready...

@karllark karllark merged commit 5c7feec into PAHFIT:master Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation enhancement science packs Features by science area (e.g., exgal, agb, etc) testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants