Aperture photometry plugin reflects display unit selection

[cshanahan1]

Update all display and outputs of aperture photometry app to reflect selected display unit in cubeviz.
The output photometry table now has a separate column for Unit.

[codecov]

Codecov Report

Attention: Patch coverage is 89.43089% with 13 lines in your changes missing coverage. Please review.

Project coverage is 88.82%. Comparing base (7826a99) to head (8385dcf).
Report is 125 commits behind head on main.

Files with missing lines	Patch %	Lines
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	89.43%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3118      +/-   ##
==========================================
+ Coverage   88.78%   88.82%   +0.03%     
==========================================
  Files         112      112              
  Lines       17446    17550     +104     
==========================================
+ Hits        15489    15588      +99     
- Misses       1957     1962       +5     

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

[camipacifici]

Looks good! I just noticed a couple of things:

• with some units the numbers can be very large or very small. Can you please use scientific notation for the min/max/ave/etc in the table? They all come out as zeros with very small/big numbers now.
• at the end of the table, the flux_scaling is not updating to follow the unit change.
• i created a subset and run aperture photometry. the plot is labeled x/y. rerunning it, the plot gets the correct units.
• then I changed units and rerun aperture photometry without changing anything else and the plot kept the points from before and updated only the line
  
  
  

[camipacifici]

Just noticed that the problem about not showing the units right away is also in the released version.

[pllim]

Now that you mention the radial plot, I wonder how the Gaussian1D fit would behave as well.

[cshanahan1]

I tested the gaussian fit and the plotting and it was working for me, but maybe something is getting out of sync. looking into it

[camipacifici]

I think the raw profile is not updating and only the label is updating. After changing the units, there is a big difference between the raw and average although they should be comparable.

[cshanahan1]

the latest commit should fix the issue with the raw radial profile plot

[camipacifici]

The fix worked, thank you!

[cshanahan1]

a few things:

1. There is no current test coverage for batch mode in cubeviz. I have run into occasional errors testing the unit conversion with batch mode, but also sometimes it does work. I started working on adding some test coverage for batch mode specifically (both generally and testing before/after unit conversion) but there are some existing issues making this complicated. For example, an error is thrown when you try to toggle on batch mode by setting plugin.multiselect to True (which works in imviz), which then doesn't allow you to select multiple apertures or use the 'unpack_batch_options' method. I think that validating that batch mode works and is covered in tests warrants its own ticket outside of this work, and that not testing batch mode + unit conversion shouldn't hold up merging this PR.

2. Similarly, there is no current test coverage for radial profile or curve of growth plots in the plugin. I looked at the markers plugin to get the syntax to access points directly from a figure to test what is happening in the UI (rather than just calling _curve_of_growth, which wouldn't pick up things in the plugin like unit changes, and is already tested on its own in the imviz tests), and that is not working here. Even if I can get it to work, I think that this goes out of scope since those basic tests don't exist already. I propose addressing this in the same followup that adds test coverage for batch mode, and adding a test there that covers unit conversion.

[pllim]

I thought we agree to disallow magnitude in aperture photometry? This is probably wrong and should not be allowed. (Kyle said this is now a separate ticket because we're going to disable mag app-wide.)

[pllim]

1. I don't know what the flux scaling mean in unit that isn't a variant of Jy. The original ask from user was to hardcode 3631 Jy (ref: https://en.wikipedia.org/wiki/AB_magnitude) as a convenient shortcut to quickly get AB mag for JWST data. Maybe it is cleaner to set it to 1 for something that isn't Jy (or some prefix of it)?
2. Not sure what is going on with the Curve of Growth JSON message. Never mind. I introduced that bug. I fixed it in a second commit that I pushed.

[pllim]

@kecnry also said we should disable multi-select for aperture photometry in Cubeviz. Is that part of this PR or a different ticket?

[kecnry]

@kecnry also said we should disable multi-select for aperture photometry in Cubeviz. Is that part of this PR or a different ticket?

This is already a separate ticket, and I just said disabling could be an option if it isn't an easy fix (but that we probably should try to get it working and covered in tests if there aren't too many roadblocks).

[bmorris3]

First comment from a first glance. More comments tomorrow!

[pllim]

As it is, background and flux_scaling is going to crash whenever you try to convert from per-wavelength to per-frequency because that conversion needs u.spectral_density that needs a wavelength/frequency as input. Example traceback:

File aper_phot_simple.py:184, in SimpleAperturePhotometry._on_display_units_changed(self, event)
    181     new_unit = u.Unit(self.display_flux_or_sb_unit)
    183     bg = self.background_value * prev_unit
--> 184     self.background_value = bg.to_value(new_unit)
    186 # convert flux scaling to new unit
    187 if self.flux_scaling is not None:

UnitConversionError: 'MJy / sr' (surface brightness) and 'erg / (Angstrom s sr cm2)' (surface brightness wav) are not convertible

For Cubeviz, you can probably grab this value from slice. But this is going to be much trickier if you ever want to enable the same functionality for Imviz because then you need to infer that value from some bandpass information that might or might not be available.

[cshanahan1]

oh, i was working on adding the equivalencies but it looks like you just committed those changes?

[pllim]

I think it is okay to not convert biweight midvariance when it fails (it will if you convert per-wave to per-freq because it is flux^2). I don't think anyone is going to notice. Good enough for now.

[pllim]

Re: #3118 (comment)

Oh ooops... Feel free to drop my commits then if you want to push yours out.

[pllim]

Actually maybe the difference is caused by #3133

(Yes, I confirmed it locally by comparing test result before/after rebase. So I updated the test result.)

[bmorris3]

This looks good to me, thanks for your perseverance @cshanahan1. It's unfortunate that the units astronomers love are so messy, but I think this makes it clearer what we're doing with them.