Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Units cleanup #3204

Merged
merged 6 commits into from
Sep 30, 2024
Merged

Units cleanup #3204

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
127d295
defined pix2 unit
cshanahan1 Sep 26, 2024
b8d5a82
define supported units in one place
cshanahan1 Sep 26, 2024
5945959
code style
cshanahan1 Sep 26, 2024
c805453
new test file
cshanahan1 Sep 27, 2024
3250230
added test
cshanahan1 Sep 27, 2024
f781b6f
Merge branch 'main' into units_cleanup
cshanahan1 Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 11 additions & 26 deletions jdaviz/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,10 @@
from jdaviz.utils import (SnackbarQueue, alpha_index, data_has_valid_wcs, layer_is_table_data,
MultiMaskSubsetState, _wcs_only_label, flux_conversion,
spectral_axis_conversion)
from jdaviz.core.validunits import check_if_unit_is_per_solid_angle
from jdaviz.core.validunits import (check_if_unit_is_per_solid_angle,
combine_flux_and_angle_units,
locally_defined_flux_units,
supported_sq_angle_units)

__all__ = ['Application', 'ALL_JDAVIZ_CONFIGS', 'UnitConverterWithSpectral']

Expand All @@ -72,32 +75,14 @@ class UnitConverterWithSpectral:
def equivalent_units(self, data, cid, units):
if cid.label == "flux":
eqv = u.spectral_density(1 * u.m) # Value does not matter here.
list_of_units = set(list(map(str, u.Unit(units).find_equivalent_units(
include_prefix_units=True, equivalencies=eqv)))
+ [
'Jy', 'mJy', 'uJy', 'MJy',
'W / (Hz m2)', 'eV / (Hz s m2)',
'erg / (Hz s cm2)', 'erg / (Angstrom s cm2)',
'ph / (Angstrom s cm2)', 'ph / (Hz s cm2)',
'ct'
]
+ [
'Jy / sr', 'mJy / sr', 'uJy / sr', 'MJy / sr',
'W / (Hz sr m2)', 'eV / (Hz s sr m2)',
'erg / (s sr cm2)', 'erg / (Hz s sr cm2)',
'erg / (Angstrom s sr cm2)',
'ph / (Angstrom s sr cm2)', 'ph / (Hz s sr cm2)',
'ct / sr'
]
+ [
'Jy / pix2', 'mJy / pix2', 'uJy / pix2', 'MJy / pix2',
'W / (Hz m2 pix2)', 'eV / (Hz s m2 pix2)',
'erg / (s cm2 pix2)', 'erg / (Hz s cm2 pix2)',
'erg / (Angstrom s cm2 pix2)',
'ph / (Angstrom s cm2 pix2)', 'ph / (Hz s cm2 pix2)',
'ct / pix2'
]
all_flux_units = locally_defined_flux_units() + ['ct']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason not to include ct (perhaps optionally) in the function?

Copy link
Contributor Author

@cshanahan1 cshanahan1 Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep it clear that 'locally_defined_flux_units' are units that can be converted to and from one another, and counts can not.

(This is a tangent but I wanted to write down my thoughts before I forget):- locally_defined_flux_units is the list of units that appears in the dropdown if your loaded data is compatible with one of the units in that list. A more descriptive name might make sense, especially to make it clear that this list of units is the available conversions for data loaded in a compatible unit, and is not relevant otherwise.

If your loaded data is in counts, and you wanted to be able to convert from counts to e/s (not sure if this actually makes sense, just an example), then there would be another list of 'flux units' that is ['counts', 'e/s'] etc.

Eventually we could use these lists of units we support and support conversions between to check the input against and raise warnings if you load a nonsense unit. We could also do away with the 'create_flux_equivalencies_list` function and just check if the input unit is compatible with one of these 'locally_defined_flux_units' lists, and append the input unit as a choice if it is (and if not, there will be no other choices for flux conversion so this will avoid the issue where it provides those units and the conversion breaks)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for the explanation. The flux choices in unit conversion is still populated by create_flux_equivalencies_list - should that be changed here/now or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its going to require a little more thought and writing some more tests to make sure different loaded units get the correct unit choices, so maybe not in this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good - deal!

angle_units = supported_sq_angle_units()
all_sb_units = combine_flux_and_angle_units(all_flux_units, angle_units)

# list of all possible units for spectral y axis, independent of data loaded
#
list_of_units = set(list(map(str, u.Unit(units).find_equivalent_units(
include_prefix_units=True, equivalencies=eqv))) + all_flux_units + all_sb_units
)
else: # spectral axis
# prefer Hz over Bq and um over micron
Expand Down
9 changes: 5 additions & 4 deletions jdaviz/configs/cubeviz/plugins/parsers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from astropy.wcs import WCS
from specutils import Spectrum1D, SpectralAxis

from jdaviz.core.custom_units import PIX2
from jdaviz.core.registries import data_parser_registry
from jdaviz.core.validunits import check_if_unit_is_per_solid_angle
from jdaviz.utils import standardize_metadata, PRIHDR_KEY, download_uri_to_path
Expand Down Expand Up @@ -195,9 +196,9 @@
# convert flux and uncertainty to per-pix2 if input is not a surface brightness
if apply_pix2:
if not check_if_unit_is_per_solid_angle(flux.unit):
flux = flux / (u.pix * u.pix)
flux = flux / PIX2
if uncertainty is not None:
uncertainty = uncertainty / (u.pix * u.pix)
uncertainty = uncertainty / PIX2

Check warning on line 201 in jdaviz/configs/cubeviz/plugins/parsers.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/configs/cubeviz/plugins/parsers.py#L201 

Added line #L201 was not covered by tests

# handle scale factors when they are included in the unit
if not np.isclose(flux.unit.scale, 1.0, rtol=1e-5):
Expand Down Expand Up @@ -606,14 +607,14 @@

# convert flux, which is always populated
flux = getattr(spectrum, 'flux')
flux = flux / (u.pix * u.pix)
flux = flux / PIX2

# and uncerts, if present
uncerts = getattr(spectrum, 'uncertainty')
if uncerts is not None:
# enforce common uncert type.
uncerts = uncerts.represent_as(StdDevUncertainty)
uncerts = StdDevUncertainty(uncerts.quantity / (u.pix * u.pix))
uncerts = StdDevUncertainty(uncerts.quantity / PIX2)

# create a new spectrum 1d with all the info from the input spectrum 1d,
# and the flux / uncerts converted from flux to SB per square pixel
Expand Down
14 changes: 8 additions & 6 deletions jdaviz/configs/cubeviz/plugins/tests/test_cubeviz_aperphot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
from numpy.testing import assert_allclose
from regions import RectanglePixelRegion, PixCoord

from jdaviz.core.custom_units import PIX2


def test_cubeviz_aperphot_cube_orig_flux(cubeviz_helper, image_cube_hdu_obj_microns):
cubeviz_helper.load_data(image_cube_hdu_obj_microns, data_label="test")
flux_unit = u.Unit("1E-17 erg*s^-1*cm^-2*Angstrom^-1*pix^-2") # actually a sb
solid_angle_unit = u.pix * u.pix
solid_angle_unit = PIX2

aper = RectanglePixelRegion(center=PixCoord(x=1, y=2), width=3, height=5)
cubeviz_helper.load_regions(aper)
Expand Down Expand Up @@ -99,7 +101,7 @@ def test_cubeviz_aperphot_cube_orig_flux(cubeviz_helper, image_cube_hdu_obj_micr
def test_cubeviz_aperphot_generated_3d_gaussian_smooth(cubeviz_helper, image_cube_hdu_obj_microns):
cubeviz_helper.load_data(image_cube_hdu_obj_microns, data_label="test")
flux_unit = u.Unit("1E-17 erg*s^-1*cm^-2*Angstrom^-1*pix^-2") # actually a sb
solid_angle_unit = u.pix * u.pix
solid_angle_unit = PIX2

gauss_plg = cubeviz_helper.plugins["Gaussian Smooth"]._obj
gauss_plg.mode_selected = "Spatial"
Expand Down Expand Up @@ -133,7 +135,7 @@ def test_cubeviz_aperphot_generated_3d_gaussian_smooth(cubeviz_helper, image_cub
assert_quantity_allclose(row["slice_wave"], 4.894499866699333 * u.um)


@pytest.mark.parametrize("cube_unit", [u.MJy / u.sr, u.MJy, u.MJy / (u.pix*u.pix)])
@pytest.mark.parametrize("cube_unit", [u.MJy / u.sr, u.MJy, u.MJy / PIX2])
def test_cubeviz_aperphot_cube_sr_and_pix2(cubeviz_helper,
spectrum1d_cube_custom_fluxunit,
cube_unit):
Expand Down Expand Up @@ -173,7 +175,7 @@ def test_cubeviz_aperphot_cube_sr_and_pix2(cubeviz_helper,
# so we can directly compare. this shouldn't be populated automatically,
# which is checked above
plg.flux_scaling = 0.003631
solid_angle_unit = u.pix * u.pix
solid_angle_unit = PIX2
cube_unit = u.MJy / solid_angle_unit # cube unit in app is now per pix2

plg.vue_do_aper_phot()
Expand All @@ -185,7 +187,7 @@ def test_cubeviz_aperphot_cube_sr_and_pix2(cubeviz_helper,
# (15 - 10) MJy/sr x 1 sr, will always be MJy since solid angle is multiplied out
assert_allclose(row["sum"], 5.0 * u.MJy)

assert_allclose(row["sum_aper_area"], 1 * (u.pix * u.pix))
assert_allclose(row["sum_aper_area"], 1 * PIX2)

# we forced area to be one sr so MJy / sr and MJy / pix2 gave the same result
assert_allclose(row["pixarea_tot"], 1.0 * solid_angle_unit)
Expand Down Expand Up @@ -226,7 +228,7 @@ def test_cubeviz_aperphot_cube_orig_flux_mjysr(cubeviz_helper, spectrum1d_cube_c
assert_allclose(row["xcenter"], 3 * u.pix)
assert_allclose(row["ycenter"], 1 * u.pix)
assert_allclose(row["sum"], 1.1752215e-12 * u.MJy) # (15 - 10) MJy/sr x 2.3504431e-13 sr
assert_allclose(row["sum_aper_area"], 1 * (u.pix * u.pix))
assert_allclose(row["sum_aper_area"], 1 * PIX2)
assert_allclose(row["pixarea_tot"], 2.350443053909789e-13 * u.sr)
assert_allclose(row["aperture_sum_mag"], 23.72476627732448 * u.mag)
assert_allclose(row["mean"], 5 * (u.MJy / u.sr))
Expand Down
Loading