Area Gauss/Drude

[Dhruvil2911]

In this PR, I've used area (power) as the parameter instead of the amplitude to fit Gaussian and Drude profiles. I've made 2 classes AreaGaussian and AreaDrude in the component_models file corresponding to each profile. There are corresponding changes in the helpers, base, and feature_strength files. I have also created an ipac file with the area columns (scipack_ExGal_area_1.ipac). The command > run_pahfit M101_Nucleus_irs.ipac scipack_ExGal_area_1.ipac will demonstrate the changes.

For feature_strengths of Gaussian/Drude, I have simply equated it to the area (power).

[karllark]

Is the idea here to have pahfit only do line fitting based on area? Or are we going to still allow central intensity? If the former, then this PR is in good shape. If the latter, then significant changes needed to support both.

The tests are failing as they are based on fitting the central intensity of lines. This can be changes to use areas instead, but need to know the answer to my above queries to know the direction to go.

[Dhruvil2911]

Thanks for reviewing the PR Karl. I've made the requested changes and added the docstring. Let me know if any other changes are required.

[karllark]

Is the idea here to have pahfit only do line fitting based on area? Or are we going to still allow central intensity? If the former, then this PR is in good shape. If the latter, then significant changes needed to support both.

The tests are failing as they are based on fitting the central intensity of lines. This can be changes to use areas instead, but need to know the answer to my above queries to know the direction to go.

Any response to this query? @jdtsmith @els1

[jdtsmith]

The new model will use power exclusively for lines and dust features. So it’s a good idea IMO to get the current astropy model to as well. I think @Ameek-Sidhu plans to just use amplitude for now as a stopgap.

[karllark]

The new model will use power exclusively for lines and dust features. So it’s a good idea IMO to get the current astropy model to as well. I think @Ameek-Sidhu plans to just use amplitude for now as a stopgap.

To be clear, this means we should change the tests to use power instead of amplitude, right? (Checking as I don't understand the "astropy model" in this context.) If so, this should be done in this PR and then all future work has to be done in power. I provide guidance on how to do this.

[jdtsmith]

The new model will use power exclusively for lines and dust features. So it’s a good idea IMO to get the current astropy model to as well. I think @Ameek-Sidhu plans to just use amplitude for now as a stopgap.

To be clear, this means we should change the tests to use power instead of amplitude, right? (Checking as I don't understand the "astropy model" in this context.) If so, this should be done in this PR and then all future work has to be done in power. I provide guidance on how to do this.

Yes, once implemented. Actually all the tests need some attention I think (haven't looked closely at them).

[karllark]

Actually all the tests need some attention I think (haven't looked closely at them).

Tests always need more. But they are working at this point until this change is made. Hence the tests can be updated as part of this PR. Then once it is merged, the tests will continue to work and do what we want - check that nothing has broken the core functionality.

[jdtsmith]

Maybe my impressions are wrong but I've been getting test failure emails for months. With apologies for my contributions to the increased entropy, I'm happy for any additional commits/PRs that help with the testing situation. I'll leave this one to you and @Dhruvil2911; but ideally this PR would go in after @Ameek-Sidhu and @drvdputt get YAML-hookup + API cleanup.

[karllark]

Briefly, the tests passed. See https://github.com/PAHFIT/pahfit/actions. ;-) Takes vigilance to keep them passing. BTW, I turned off email notification of failing tests long, long ago. Just too many!

[karllark]

Note that the tests are currently failing due to codestyle issues. We have quite a suite of tests to make sure all parts of the repository work including the code, docs, codestyle, etc.

[jdtsmith]

@karllark @alexmaragko @drvdputt Do we intend to follow through with this? No matter what happens with the new model, it seems smart to have an astropy.modeling version "along for the ride" for testing (and as a backup). For that to work "for real", we need a power-based Gaussian and Drude. Right now we are lying and pretending amplitude = power.

[karllark]

Yes this can be done fairly easily.

[jdtsmith]

Great thanks @karllark ; would appreciate checking and cleaning up this PR (or generating a new one). I believe we're close to merging the dev branch, so it could happen after that. Then at least we could do our power-based fits.

[jdtsmith]

Based on discussions with @drvdputt, we need to clarify the astropy-flavored pahfit model's internal units to complete this. This is TBD until dev merge.

[jdtsmith]

This has been superseded by #295, continue conversation there.