BUG: Fix get_model_parameters for Gaussian1D

[pllim]

Description

This pull request is to remove special unit handling for Gaussian model because it breaks viz.get_model_parameters. Not sure why they were added but maybe it was needed at some point but it is no longer the case.

I cannot find any unit tests for the Model Fitting plugin, that this method is affected by. This method is also not in any other existing tests. So maybe adding a test for this fix is out of scope... 😬

Fixes #975

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

• Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
• Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
• Do the proposed changes follow the STScI Style Guides?
• Are tests added/updated as required? If so, do they follow the STScI Style Guides?
• Are docs added/updated as required? If so, do they follow the STScI Style Guides?
• Did the CI pass? If not, are the failures related?
• Is a change log needed? If yes, is it added to CHANGES.rst?
• Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[codecov]

Codecov Report

Merging #976 (34c8caf) into main (e338e9b) will increase coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #976      +/-   ##
==========================================
+ Coverage   70.06%   70.10%   +0.04%     
==========================================
  Files          71       71              
  Lines        5121     5118       -3     
==========================================
  Hits         3588     3588              
+ Misses       1533     1530       -3     

Impacted Files	Coverage Δ
jdaviz/core/helpers.py	18.55% <0.00%> (+0.55%)	⬆️

---

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 e338e9b...34c8caf. Read the comment docs.

[ibusko]

Looks like unit tests that would cause the old code to crash but the new one to pass, would be useful here. Any idea on what was the particular case that caused the Gaussian fit to crash?

[pllim]

Since there is zero test right now, trying to write up a sensible test would take like 5-10x as long as writing the fix... 👀

[pllim]

The case is explained in #975

[rosteen]

Confirmed that it was broken, and now it's not! Thanks 👍

[rosteen]

I think there are tickets open to add unit testing for some of the plugins where it's missing already, so I support that being a separate effort from this fix.

[kecnry]

Was this caused by a change in astropy (do we need to worry about older versions of astropy still relying on the old if-statement)?

[pllim]

astropy.modeling has been supporting units since v2.0 (astropy/astropy#4855). And the current LTS is v5.0. So, I don't think we need to worry about the old versions?

[pllim]

Still, doesn't hurt to do a test with this using astropy 4.3.x

jdaviz/setup.cfg

Line 21 in e338e9b

astropy>=4.3

[pllim]

Seems to work with astropy 4.3:

>>> specviz.get_model_parameters()
{'Model':
 {'amplitude_0': <Quantity 29.58504463 1e-17 erg / (Angstrom cm2 s)>,
  'mean_0': <Quantity 6858.37344117 Angstrom>,
  'stddev_0': <Quantity 63.64975782 Angstrom>,
  'amplitude_1': <Quantity 54.63609656 1e-17 erg / (Angstrom cm2 s)>}}

>>> cubeviz.get_model_parameters()
{'Model':
 {'slope': <Quantity 1364993.81048269 1e-17 erg / (Angstrom cm2 m s)>,
  'intercept': <Quantity 0.74306448 1e-17 erg / (Angstrom cm2 s)>},
 'Model_3d': {
  'slope': <Quantity [[-1.831916e-318, -1.831916e-318, -1.831916e-318, ...,
              -1.831916e-318, -1.831916e-318, -1.831916e-318],
             [-1.831916e-318, -1.831916e-318, -1.831916e-318, ...,
              -1.831916e-318, -1.831916e-318, -1.831916e-318],
             [-1.831916e-318, -1.831916e-318, -1.831916e-318, ...,
              -1.831916e-318, -1.831916e-318, -1.831916e-318],
             ...,
             [-1.831916e-318, -1.831916e-318, -1.831916e-318, ...,
              -1.831916e-318, -1.831916e-318, -1.831916e-318],
             [-1.831916e-318, -1.831916e-318, -1.831916e-318, ...,
              -1.831916e-318, -1.831916e-318, -1.831916e-318],
             [-1.831916e-318, -1.831916e-318, -1.831916e-318, ...,
              -1.831916e-318, -1.831916e-318, -1.831916e-318]] 1e-17 erg / (Angstrom cm2 m s)>,
  'intercept': <Quantity [[5.e-324, 5.e-324, 5.e-324, ..., 5.e-324, 5.e-324, 5.e-324],
             [5.e-324, 5.e-324, 5.e-324, ..., 5.e-324, 5.e-324, 5.e-324],
             [5.e-324, 5.e-324, 5.e-324, ..., 5.e-324, 5.e-324, 5.e-324],
             ...,
             [5.e-324, 5.e-324, 5.e-324, ..., 5.e-324, 5.e-324, 5.e-324],
             [5.e-324, 5.e-324, 5.e-324, ..., 5.e-324, 5.e-324, 5.e-324],
             [5.e-324, 5.e-324, 5.e-324, ..., 5.e-324, 5.e-324, 5.e-324]] 1e-17 erg / (Angstrom cm2 s)>}}

[kecnry]

Much cleaner and since it seems to work on our required minimum version of astropy, I think this is good to merge!