Add more flux units for SB conv

[pllim]

Description

This pull request is to address broken spectrum viewer for MANGA cube when Surface Brightness is selected and user wants to change flux units.

I threw in a fix for the reporting "counts cube" but not sure how to test that because that detail was not given in JIRA.

Change log entry

• Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
  list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
  should add a no-changelog-entry-needed label.

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 milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

[pllim]

If the unrelated changes bother you, I can revert them. Just happened to come across them while trying to make sense of the test failure.

[pllim]

Too many parenthesis so I removed some. No functional change.

[gibsongreen]

From what was discussed at tag up, I'll mention @cshanahan1 has a ticket that consolidates the flux_conversion here, and flux_conversion_general function so there will be some refactoring down the line

[pllim]

So, should I revert?

[gibsongreen]

I don't think so, just wanted to mention it here. The plan is to remove one of the two functions (I'm not sure which), none of the changes are major conflicts here and it may just get removed entirely so I'm good with keeping the changes here for now.

[pllim]

CI failures are unrelated. They also fail on main.

[gibsongreen]

Tested this on a bunch of cubes, behavior is now fixed and match expected behavior from recent discussions. I think this can be marked as trivial but up to you, I'll tag @cshanahan1 here again just so she knows about the non-functional changes to flux_converision

[pllim]

Thanks for the review! Sure, I marked it as trivial but I cannot merge until the DQ failure is fixed (#3390) unless admin overrides the branch protection.

[cshanahan1]

'Indirect_units' seem to be for flux units that can't be directly translated to surface brightness when a cube is in Jy because they require both a custom equivalency and u.spectral_density. However, if a cube is in erg/s/cm2/A then wouldn't the 'indirect_units' list be comprised of Jy/MJy/uJy etc instead? There should be two sets of 'indirect_units' since that will change based on what the unit in question is, and if it needs u.spectral? If we have all surface brightness units in this list of units (as this PR introduces) then that kind of makes it unnecessary to define these translations as 'indirect' since they may not be. I also don't quite understand why only PIX2 SB units were added here, don't we pass PIXAR_SR to this flux conversion function (or look for it in meta)? It doesn't make sense that the existing units can either be per PIX2 or Sr but not these new units just added

I am doing some work to try to use the flux_conversion_general function for all unit conversions so I think if this fixes a pressing issue its fine since I will refactor this anyway. Could you make sure to add a test case so I can make sure that this works with my refactor?

[pllim]

What should I be testing? There is not traceback or warning or anything. Just visually things look off in the spectrum viewer. But when I access viewer.figure.marks, there are so many things going on there that I am not sure what to look for? Can you please advise? Thanks.

[pllim]

@cshanahan1 those are all good questions. I think the unit conversion stuff got too complicated. I was simply looking for the least intrusive way to fix the reported issue while resisting the urge to refactor everything.

[pllim]

p.s. All the callback and async stuff not helping either.

[pllim]

p.p.s. Speaking of async, this also bothered me a bit, the fact that a method is purposely called twice when it does not really have to be this way but it is a battle for another day.

jdaviz/jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py

Line 315 in 3107b87

# NOTE: setting sb_unit_selected will call this method again with axis=='sb',

[cshanahan1]

What should I be testing? There is not traceback or warning or anything. Just visually things look off in the spectrum viewer. But when I access viewer.figure.marks, there are so many things going on there that I am not sure what to look for? Can you please advise? Thanks.

I'm not familiar with the ticket that spawned this PR, but writing a test that loads a cube in these units and does a translation and tests the viewer marks or limits to make sure the unit conversion was done seems reasonable?

[pllim]

tests the viewer marks

How do I extract the correct marks?

[cshanahan1]

You could also add a test to test_spec_sb_flux_conversion() that just tests the flux conversion function

I'm confused why cts was added, this cant be translated to any other flux unit, and it should be 'directly' convertible between flux and SB using one of the equivalencies we define

[pllim]

I'm confused why cts was added, this cant be translated to any other flux unit, and it should be 'directly' convertible between flux and SB using one of the equivalencies we define

Gilbert has a special JWST cube from Cami with that unit and it was mentioned in the ticket.

Which equivalencies is that? I thought the indirect unit stuff was it. Adding it there did fix the bug for counts, as Gilbert tested.

[cshanahan1]

Which equivalencies is that? I thought the indirect unit stuff was it. Adding it there did fix the bug for counts, as Gilbert tested.

_eqv_pixar_sr and _eqv_flux_to_sb_pixel. 'indirect' means they need one of these and u.spectral for conversion, which can't be combined.

Another thought - this PR fixes spectral axis translation for whatever dataset was broken, but won't fix plugin unit conversion since that goes through flux_conversion_general. My ticket to consolidate these will address that, but we should make sure to revisit this case in that PR

Sorry this is all so confusing, hopefully this will be simplified soon

[pllim]

Yeah I am not sure why _eqv_flux_to_sb_pixel is not accessed with the Unit Conversion drop down is selected at "Surface Brightness" and I got lost in all the different ways units are being handled throughout the app. All I know is this patch somehow fixes the use case reported.

I tried to add this test as you requested but it fails even with this patch, so as you suspected, whatever logic route this workflow is taking does not go through the way you wanted it to.

If you really don't like this patch, we can just close this PR without merge, I will unassign myself and then you can see if your refactor would automatically fix it later. 🤷‍♀️

def test_spec_sb_flux_conversion_no_pixscale():
    # Actual spectrum content does not matter, just the meta is used here.
    spec = Spectrum1D(flux=[1, 1, 1] * u.Jy, spectral_axis=[1, 2, 3] * u.um)

    # values != 2
    values = [10, 20, 30]

    # conversions with eq pixels
    assert_allclose(flux_conversion(values, u.Jy / PIX2, u.Jy, spec), [10, 20, 30])
    assert_allclose(flux_conversion(values, u.Jy, u.Jy / PIX2, spec), [10, 20, 30])

    assert_allclose(flux_conversion(values, u.ct / PIX2, u.ct, spec), [10, 20, 30])
    assert_allclose(flux_conversion(values, u.ct, u.ct / PIX2, spec), [10, 20, 30])

    # complex translation Jy / PIX2 -> erg / (Angstrom s cm2 PIX2)
    targ = [2.99792458e-12, 1.49896229e-12, 9.99308193e-13]
    assert_allclose(flux_conversion(values, u.Jy / PIX2, u.erg / (u.Angstrom * u.s * u.cm**2 * PIX2), spec), targ)  # noqa: E501

    # complex translation erg / (Angstrom s cm2 PIX2) -> Jy / PIX2
    targ = [3.33564095e+13, 2.66851276e+14, 9.00623057e+14]
    assert_allclose(flux_conversion(values, u.erg / (u.Angstrom * u.s * u.cm**2 * PIX2), u.Jy / PIX2, spec), targ)  # noqa: E501

    spectral_values = spec.spectral_axis
    spec_unit = u.MJy
    eqv = u.spectral_density(spectral_values)

    # test spectrum when target unit in untranslatable unit list
    target_values = [5.03411657e-05, 2.01364663e-04, 4.53070491e-04]
    expected_units = u.ph / (u.Hz * u.s * u.cm**2)
    returned_values, return_units, unit_flag = _indirect_conversion(
        values=values, orig_units=u.MJy,
        targ_units=u.ph / (u.s * u.cm**2 * u.Hz * PIX2),
        eqv=eqv,
        spec_unit=spec_unit,
        indirect_needs_spec_axis=None
    )
    assert_allclose(returned_values, target_values)
    assert return_units == expected_units
    assert unit_flag == 'orig'

[gibsongreen]

'Indirect_units' seem to be for flux units that can't be directly translated to surface brightness when a cube is in Jy because they require both a custom equivalency and u.spectral_density. However, if a cube is in erg/s/cm2/A then wouldn't the 'indirect_units' list be comprised of Jy/MJy/uJy etc instead? There should be two sets of 'indirect_units' since that will change based on what the unit in question is, and if it needs u.spectral? If we have all surface brightness units in this list of units (as this PR introduces) then that kind of makes it unnecessary to define these translations as 'indirect' since they may not be. I also don't quite understand why only PIX2 SB units were added here, don't we pass PIXAR_SR to this flux conversion function (or look for it in meta)? It doesn't make sense that the existing units can either be per PIX2 or Sr but not these new units just added

Going from erg/s/cm2/A to erg/s/cm2/A/PIX2 and then converting again to Jy/PIX2 would require the spectral density equivalencies and the custom equivalencies, so the indirect conversion doesn’t only apply to the Jy/similar units.

The context of the ticket is for counts cubes and manga cubes both of which will be in PIX2, so the PIXAR_SR keyword wouldn’t apply in either of these cases.

I don’t think we would need 2 separate indirect units lists here, the logic for the indirect conversion is only going to get hit when the original and target units can’t be directly converted, otherwise it just returns.

[pllim]

As Clare requested, I change this back to draft for now as her refactor would remove the patched function entirely.

Gilbert is looking into how to access the marks for test.