-
Notifications
You must be signed in to change notification settings - Fork 76
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.
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
Perform corrections to Line Analysis Flux Results #1564
Changes from all commits
88cfc86
e74c0e4
5a3cad4
ab3063a
0438506
a118e86
cac64a6
54b98c4
982a377
e762634
a71301c
f44aa0b
31d2f7b
549299f
9301ec4
cae45bf
d6d5e85
316af5c
ffed328
972a1f4
1c32f30
ac16fd2
23a9f7e
4a7cf97
320f91a
61debce
fda9b00
1aeca73
a4d8ea7
fbbffa5
1e7121a
f6a2529
99fb4fc
c2e1e49
9d34a52
7ac6af8
9380d31
01f60bc
eee47e0
ebbe983
c360db4
f9c513c
31ab2ee
5631edc
f9589a6
1557d16
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -136,7 +136,7 @@ def test_continuum_surrounding_spectral_subset(specviz_helper, spectrum1d): | |
plugin.width = 3 | ||
|
||
# Values have not yet been validated | ||
np.testing.assert_allclose(float(plugin.results[0]['result']), 350.89288537581467, atol=1e-5) | ||
np.testing.assert_allclose(float(plugin.results[0]['result']), 2.153181e-13, atol=1e-15) | ||
Comment on lines
138
to
+139
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we're doing line flux unit conversion to all data, not just those from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where science validation becomes important. Have the POs also test this plugin with some non-JWST data and see if results are correct. On our side:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, do you understand this change? What was the unit before, what is the unit now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is definitely a PR that requires PO review, so I agree for the first, and last, point. Though I wonder whether a full scientific integrity test suite should be a blocker for this PR or not? As for your other point, the main solace I can provide for the "arbitrary data" is that the conversion only happens when the units are specifically in the right bases. Could a user theoretically provide some other data that happens to have "the same unit" but mean something different? I don't know There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, if we want full QA, this is never going to go in. So I would say no, but we first need to understand why our existing test results have changed and is the change correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pllim (1) Regarding arbitrary units, these shouldn't be touched by this PR. Specutils should know how to handle the units commonly used by astronomers: Jy, erg/s/cm^2/A, etc..., regardless of what instrument/telescope is used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here are the only two cases for spectra with physical units: |
||
|
||
|
||
def test_continuum_spectral_same_value(specviz_helper, spectrum1d): | ||
|
@@ -215,7 +215,7 @@ def test_continuum_subset_spectral_entire(specviz_helper, spectrum1d): | |
plugin.width = 3 | ||
|
||
# Values have not yet been validated | ||
np.testing.assert_allclose(float(plugin.results[0]['result']), -467.6854635447396, atol=1e-5) | ||
np.testing.assert_allclose(float(plugin.results[0]['result']), -2.79572e-13, atol=1e-15) | ||
|
||
|
||
def test_continuum_subset_spectral_subset2(specviz_helper, spectrum1d): | ||
|
@@ -248,7 +248,7 @@ def test_continuum_subset_spectral_subset2(specviz_helper, spectrum1d): | |
plugin.width = 3 | ||
|
||
# Values have not yet been validated | ||
np.testing.assert_allclose(float(plugin.results[0]['result']), 32.520521, atol=1e-5) | ||
np.testing.assert_allclose(float(plugin.results[0]['result']), 1.482418e-14, atol=1e-16) | ||
|
||
|
||
def test_continuum_surrounding_no_right(specviz_helper, spectrum1d): | ||
|
@@ -276,7 +276,7 @@ def test_continuum_surrounding_no_right(specviz_helper, spectrum1d): | |
plugin.width = 3 | ||
|
||
# Values have not yet been validated | ||
np.testing.assert_allclose(float(plugin.results[0]['result']), 39.76685499263615, atol=1e-5) | ||
np.testing.assert_allclose(float(plugin.results[0]['result']), 4.204513e-14, atol=1e-16) | ||
|
||
|
||
def test_continuum_surrounding_no_left(specviz_helper, spectrum1d): | ||
|
@@ -304,7 +304,7 @@ def test_continuum_surrounding_no_left(specviz_helper, spectrum1d): | |
plugin.width = 3 | ||
|
||
# Values have not yet been validated | ||
np.testing.assert_allclose(float(plugin.results[0]['result']), 146.67186446784513, atol=1e-5) | ||
np.testing.assert_allclose(float(plugin.results[0]['result']), 7.570859e-14, atol=1e-16) | ||
|
||
|
||
def test_subset_changed(specviz_helper, spectrum1d): | ||
|
@@ -335,4 +335,4 @@ def test_subset_changed(specviz_helper, spectrum1d): | |
specviz_helper.app.state.drawer = True | ||
|
||
# Values have not yet been validated | ||
np.testing.assert_allclose(float(plugin.results[0]['result']), 350.89288537581467, atol=1e-5) | ||
np.testing.assert_allclose(float(plugin.results[0]['result']), 2.153181e-13, atol=1e-15) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,123 @@ | ||
from astropy.tests.helper import assert_quantity_allclose | ||
import astropy.units as u | ||
from glue.viewers.profile.state import FUNCTIONS as COLLAPSE_FUNCTIONS | ||
import numpy as np | ||
import pytest | ||
from specutils import Spectrum1D | ||
|
||
from jdaviz import Cubeviz | ||
|
||
import warnings | ||
warnings.filterwarnings('ignore') | ||
|
||
# Maps input spectrum flux unit to expected line analysis flux unit | ||
expected_lineflux_results = { | ||
u.Jy/u.sr: u.Unit('W/(m2*sr)'), | ||
u.Jy: u.Unit('W/m2'), | ||
u.Unit('W/m2/m')/u.sr: u.Unit('W/(m2*sr)'), | ||
u.Unit('W/m2/m'): u.Unit('W/m2') | ||
} | ||
|
||
test_cases = list(expected_lineflux_results.keys()) | ||
|
||
|
||
# Test cases for unit gaussian tests | ||
def _gauss_with_unity_area(x, mean, sigma): | ||
''' Gaussian function with area = 1 unit ''' | ||
dx = x - mean | ||
n = 1/(sigma*np.sqrt(2*np.pi)) | ||
g = n * np.exp(-dx**2/(2*sigma**2)) | ||
return g | ||
|
||
|
||
mn = 1.5 | ||
sig = 0.01 | ||
|
||
# 5 cases: replace fl_wave with fnu_freq, fnu_wave, flam_wave, flam_freq, or fl_wave | ||
# to try the different cases here | ||
unit_flux_gaussian_test_cases = [] | ||
|
||
# unit-flux gaussian in frequency space | ||
freq = np.arange(1, 2, 0.001)*u.Hz | ||
flux_freq = _gauss_with_unity_area(freq.value, mn, sig)*1.0E26*u.Jy | ||
fnu_freq = Spectrum1D(spectral_axis=freq, flux=flux_freq) | ||
unit_flux_gaussian_test_cases.append(fnu_freq) | ||
fnu_wave = Spectrum1D(spectral_axis=fnu_freq.wavelength, flux=flux_freq) | ||
unit_flux_gaussian_test_cases.append(fnu_wave) | ||
|
||
# unit-flux gaussian in wavelength space | ||
lam = np.arange(1, 2, 0.001)*u.m | ||
flux_wave = _gauss_with_unity_area(lam.value, mn, sig)*1.0*u.W/u.m**2/u.m | ||
flam_wave = Spectrum1D(spectral_axis=lam, flux=flux_wave) | ||
unit_flux_gaussian_test_cases.append(flam_wave) | ||
flam_freq = Spectrum1D(spectral_axis=flam_wave.frequency, flux=flux_wave) | ||
unit_flux_gaussian_test_cases.append(flam_freq) | ||
|
||
|
||
def _calculate_line_flux(viz_helper): | ||
''' | ||
Returns the line flux calculation by opening the Line Analysis Plugin | ||
Assumes the plugin hasn't been opened yet | ||
''' | ||
# Open the plugin and force the calculation | ||
viz_helper.app.state.drawer = True | ||
line_analysis_plugin = viz_helper.app.get_tray_item_from_name('specviz-line-analysis') | ||
line_analysis_plugin.open_in_tray() | ||
# Retrieve Results | ||
for result in line_analysis_plugin.results: | ||
if result['function'] == 'Line Flux': | ||
return result | ||
|
||
|
||
@pytest.mark.filterwarnings('ignore') | ||
@pytest.mark.parametrize('spectra_fluxunit', test_cases) | ||
def test_cubeviz_collapse_fluxunits(spectrum1d_cube_custom_fluxunit, spectra_fluxunit): | ||
''' Calculates line flux and checks the units for each collapse function ''' | ||
data = spectrum1d_cube_custom_fluxunit(spectra_fluxunit) | ||
for function in COLLAPSE_FUNCTIONS: | ||
# Initialize Cubeviz with specific data and collapse function | ||
cubeviz_helper = Cubeviz() | ||
data_label = "Test Cube" | ||
cubeviz_helper.load_data(data, data_label=data_label) | ||
cubeviz_helper.app.get_viewer('spectrum-viewer').state.function = function | ||
|
||
lineflux_result = _calculate_line_flux(cubeviz_helper) | ||
|
||
autocollapsed_spectrum_unit = cubeviz_helper.specviz.get_spectra()[data_label + "[FLUX]" | ||
].flux.unit | ||
# Futureproofing: Eventually Cubeviz autocollapse will change the flux units of the | ||
# spectra depending on whether the spectrum was collapsed OVER SPAXELS or not. Only | ||
# do the assertion check if we KNOW what the expected lineflux results should be | ||
if autocollapsed_spectrum_unit in expected_lineflux_results.keys(): | ||
assert u.Unit(lineflux_result['unit']) == expected_lineflux_results[spectra_fluxunit] | ||
|
||
|
||
@pytest.mark.filterwarnings('ignore') | ||
@pytest.mark.parametrize('test_case', unit_flux_gaussian_test_cases) | ||
def test_unit_gaussian(specviz_helper, test_case): | ||
''' | ||
Test an Area 1 Gaussian and ensure the result returns in W/m2 | ||
Test provided by Patrick Ogle | ||
''' | ||
specviz_helper.load_data(test_case) | ||
|
||
lineflux_result = _calculate_line_flux(specviz_helper) | ||
assert_quantity_allclose(float(lineflux_result['result']) * u.Unit(lineflux_result['unit']), | ||
1*u.Unit('W/m2')) | ||
|
||
|
||
@pytest.mark.filterwarnings('ignore') | ||
def test_unit_gaussian_mixed_units_per_steradian(specviz_helper): | ||
''' | ||
A special unit test of Area 1 with mixed units. Should return W/m2sr | ||
Test provided by Patrick Ogle | ||
''' | ||
# unit-flux gaussian in wavelength space, mixed units, per steradian | ||
lam_a = np.arange(1, 2, 0.001)*u.Angstrom | ||
flx_wave = _gauss_with_unity_area(lam_a.value, mn, sig)*1E3*u.erg/u.s/u.cm**2/u.Angstrom/u.sr | ||
fl_wave = Spectrum1D(spectral_axis=lam_a, flux=flx_wave) | ||
|
||
specviz_helper.load_data(fl_wave) | ||
lineflux_result = _calculate_line_flux(specviz_helper) | ||
assert_quantity_allclose(float(lineflux_result['result']) * u.Unit(lineflux_result['unit']), | ||
1*u.Unit('W/(m2sr)')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hopefully this will be improved upstream some day... but does the trick for now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created astropy/specutils#980 in specutils to make sure we don't forget to discuss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my comment on these lines was referring to the fact that
.uncertainty
(or any arbitrary attribute added to a quantity) isn't supported by astropy and so is stripped from the quantity object when calling.to
(see astropy/specutils#961 for some related discussion).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh right. Sorry my comment was meant as a direct response to your general review. Sorry, poor placement