Fix cube fitting compatibility with unit conversion

[rosteen]

Opening as draft since I still need to work on the unit conversion part, but I wanted to see the diff and remind myself what I'd changed so far.

[cshanahan1]

Tested this out, I do see the fix that now the component units are always in 'surface brightness' if the cube is surface brightness, and do not change based on what is selected for the spectral y axis, so that's fixed. However, I'm still not getting any results in the table or plotted when I run a cube fit on this loaded data using one of the test fixtures to create a cube:

cube = _create_spectrum1d_cube_with_fluxunit(shape=(25, 25, 4), fluxunit=u.Jy / u.sr, with_uncerts=True)

[rosteen]

Tested this out, I do see the fix that now the component units are always in 'surface brightness' if the cube is surface brightness, and do not change based on what is selected for the spectral y axis, so that's fixed. However, I'm still not getting any results in the table or plotted when I run a cube fit on this loaded data using one of the test fixtures to create a cube:

cube = _create_spectrum1d_cube_with_fluxunit(shape=(25, 25, 4), fluxunit=u.Jy / u.sr, with_uncerts=True)

Hm, the model fit plots fine with the example notebook data, but now that you mention it I don't see table results. I'll look into it on Monday.

[rosteen]

Tested this out, I do see the fix that now the component units are always in 'surface brightness' if the cube is surface brightness, and do not change based on what is selected for the spectral y axis, so that's fixed. However, I'm still not getting any results in the table or plotted when I run a cube fit on this loaded data using one of the test fixtures to create a cube:

cube = _create_spectrum1d_cube_with_fluxunit(shape=(25, 25, 4), fluxunit=u.Jy / u.sr, with_uncerts=True)

From a comment in the code: # cube fits are currently unsupported in tables - I think that's out of scope for this. I'll investigate the test fixture not plotting.

[rosteen]

Screen.Recording.2024-09-16.at.9.46.45.AM.mov

@cshanahan1 hm, I see the model plot fine with that cube (I fit a constant to it). Are you sure you didn't need to go into the data menu and add the model to the viewer? Or did it fail to appear with the Plot in Viewer option toggled on?

I do see an error trying to add a Linear model component, trying to fix that...

[rosteen]

Note that this merging this PR on top of #3156 resolves the test failures (other than the known axis label failure there).

[cshanahan1]

still seeing some out of sync units. I loaded a Jy/sr cube, changed the flux unit to MJy, and the model fitting plugin shows Jy as the flux unit. This happened when MJy was set before creating the component for the first time, so I shouldn't have to re-estimate parameters, but even when doing that the incorrect unit remains

EDIT from Ricky: Sorry @cshanahan1 I accidentally edited this reply instead of quoting it, and lost your screenshot/recording. The danger of admin power 😬

[codecov]

Codecov Report

Attention: Patch coverage is 85.45455% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.21%. Comparing base (d49e3b5) to head (b33dcc0).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...igs/default/plugins/model_fitting/model_fitting.py	86.79%	7 Missing ⚠️
...figs/default/plugins/model_fitting/initializers.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3190      +/-   ##
==========================================
- Coverage   88.50%   88.21%   -0.29%     
==========================================
  Files         124      124              
  Lines       18448    18487      +39     
==========================================
- Hits        16327    16308      -19     
- Misses       2121     2179      +58     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[camipacifici]

Linear fitting to cube or single spectrum does not break anymore!
In this PR, I see the icon to open the model fitting table in a different window, but the link goes to a 404 window.

[rosteen]

still seeing some out of sync units. I loaded a Jy/sr cube, changed the flux unit to MJy, and the model fitting plugin shows Jy as the flux unit. This happened when MJy was set before creating the component for the first time, so I shouldn't have to re-estimate parameters, but even when doing that the incorrect unit remains

Fixed, toggling the cube fit was triggering the dataset selected logic, which was resetting the units. I removed all the unit setting from there in favor of setting it when the first component is initialized (and whenever user changes units or toggles cube fit).

[cshanahan1]

I see it now respecting flux unit selection when i change between Jy/MJy, but breaks on erg / (Angstrom s cm2 sr). Is it missing a spectral_density equivalency?

[rosteen]

I see it now respecting flux unit selection when i change between Jy/MJy, but breaks on erg / (Angstrom s cm2 sr). Is it missing a spectral_density equivalency?

Oh, likely, I think I know where....one sec. Good catch.

[cshanahan1]

Could you add a test that tests cube fitting with unit conversion(s)? Maybe something where you change from Jy > MJy, and Jy > er/s/cm2/A ?

[rosteen]

@cshanahan1 I added a test, but I won't be able to test changing the unit to erg / (Angstrom s cm2) until the contour bug is fixed.

[cshanahan1]

Tested this one final time and couldn't break it. Talked on slack with ricky about a few points of confusion i had and he cleared those up so I'll approve, looks good!

[cshanahan1]

pssst

[rosteen]

FIxed.

[gibsongreen]

Did one last look over and round of testing, all looks good to me and works as I'd expect!