Simple utility continuum baseline checker #538

brechmos-stsci · 2019-10-16T13:30:58Z

This PR will create a simple spectrum checker to see if the flux baseline is near zero as this is required for several of the other methods (e.g., find_lines which assumes the flux data is continuum subtracted).

Fixes #535

…-continuum-subtracted

eteq

Several inline suggestions.

A few broader comments:

I think the docstring needs to describe the flow of what happens given various values of eps and whether or not the uncertainty is present (but maybe you were waiting on a first pass?)
This function doesn't seem to be using the mask. Presumably the user does not want masked values to be included, so I'd say the thing to do is just s.flux[~s.mask] if a mask is present.
There needs to be some way to turn the decorator "off", which the warning message should explain. I suggest using the configuration item machinery (see http://docs.astropy.org/en/stable/config/#adding-new-configuration-items) with just a boolean config item like always_check_continuum that defaults to True but just causes the decorator to immediately return True if it's set to False. Then you tell the user to use the with version of the config item machinery to supress the check.
Once the modification for the "no uncertainty and no unit" case is in, we should test this on some realistic spectrum. Maybe use the spectrum from the "getting started" example? The goal is to check whether or not the three cases actually yield sensible answers, and also do some timing tests relative to other analysis functions like a line flux measurement.

specutils/analysis/flux.py

eteq · 2019-10-18T15:11:08Z

specutils/analysis/flux.py

+    if hasattr(spectrum, 'uncertainty') and spectrum.uncertainty:
+        return np.median(spectrum.flux / spectrum.uncertainty.quantity) < eps
+    else:
+        raise Exception('Spectrum flux has units, eps does not, either include uncertainty or use units on eps')


I think this else case should instead use the median absolute deviation (in the "scaled to be like sigma" form - https://docs.astropy.org/en/stable/api/astropy.stats.mad_std.html ) as the "uncertainty" estimate, rather than raising an error. Then the decorator can default to using that case with an eps of .01 or something like that, and we can count on this function always running on a spectrum.

(This is what makes it a heuristic for continuum subtraction - not all spectra that have their continuum subtracted really will have this property, but it's a reasonable guess for many spectra.)

Right, but, this was my question in #535 (comment).

Oops, sorry - to finish out that conversation here, I don't really understand your point there. In the case of your example linked there, that looks to have done exactly what was expected: the fluxes are close together, and that closeness indicates the "uncertainty per pixel" is probably low, so therefore the fact that the average offset from zero is larger than the scatter is exactly the sign of "continuum not subtracted" that we are looking for.

Co-Authored-By: Erik Tollerud <erik.tollerud@gmail.com>

…-stsci/specutils into i535-check-continuum-subtracted

brechmos-stsci · 2019-10-22T16:38:17Z

I think the docstring needs to describe the flow of what happens given various values of eps and whether or not the uncertainty is present (but maybe you were waiting on a first pass?)

I was waiting for the first pass to be done before finishing up the documentation.

This function doesn't seem to be using the mask. Presumably the user does not want masked values to be included, so I'd say the thing to do is just s.flux[~s.mask] if a mask is present.

Right, I was waiting to make sure the rest was good, but I added it in now.

There needs to be some way to turn the decorator "off", which the warning message should explain. I suggest using the configuration item machinery (see http://docs.astropy.org/en/stable/config/#adding-new-configuration-items) with just a boolean config item like always_check_continuum that defaults to True but just causes the decorator to immediately return True if it's set to False. Then you tell the user to use the with version of the config item machinery to supress the check.

I added this in, but you or someone will have to check it to make sure it is implemented correctly .I looked at astropy to see how it was used there, and mimiced it.

Once the modification for the "no uncertainty and no unit" case is in, we should test this on some realistic spectrum. Maybe use the spectrum from the "getting started" example? The goal is to check whether or not the three cases actually yield sensible answers, and also do some timing tests relative to other analysis functions like a line flux measurement.

Sure.

eteq

Several inline suggestions. Also, as per discussion in #535, should change the name to is_continuum_below_threshold

eteq · 2019-10-28T21:12:04Z

specutils/__init__.py

+    Configuration parameters for specutils.
+    """
+
+    always_check_continuum = _config.ConfigItem(


Instead of "always", perhaps do_continuum_function_check or something like that? Only because "always" could be taken as "for all functions" and it's really just "selected functions".

eteq · 2019-10-28T21:17:34Z

specutils/analysis/flux.py

+    spectrum : `~specutils.spectra.spectrum1d.Spectrum1D`
+        The spectrum object over which the width will be calculated.
+
+    eps: float


eps -> threshold. And also should say that it can be a flux-unit quantity or a float/dimensionless qunatity, and that determines if it's a flux threshold vs a "signal/noise" threshold

already done

eteq · 2019-10-28T21:19:07Z

specutils/analysis/flux.py

+    if uncertainty:
+        return np.median(flux / uncertainty.quantity) < eps
+    else:
+        warnings.warn('Spectrum does not appear to be continuum subtracted.', AstropyUserWarning)


I'm confused why this warning is here?

I think you saw an old PR.

eteq · 2019-10-28T21:19:51Z

specutils/analysis/flux.py

+            if check:
+                spectrum = args[0]
+                if not is_continuum_near_zero(spectrum, eps):
+                    warnings.warn('Spectrum does not appear to be continuum subtracted.',


This should also have some text that refers back to always_check_continuum and tells the user exactly what they need to do to turn it off (or links to a page in the docs that explains it).

I added a short message with a link back to the astropy configuration section.

eteq · 2019-10-28T21:21:20Z

Oops, I think my review there was against an old version, but I didn't realize there was a new commit until I had already done the review. So ignore anything that was fixed in 72446dc that's in my comments.

eteq

A couple other inline items, but overall looking pretty good now!

eteq · 2019-10-29T17:19:53Z

specutils/analysis/flux.py

+    def actual_decorator(function):
+        @wraps(function)
+        def wrapper(*args, **kwargs):
+            if check:


Hmm. I might be confused but I think this isn't quite right - I think you want to do

Suggested change

if check:

if check():

because a configuration item can be changed at runtime.

Or alternatively,

Suggested change

if check:

if check() if callable(check) else check:

which would let you pass in True/False in addition to a configuration item

Based on our conversation, this changed to an alternative form (that is correct :) ).

eteq · 2019-10-29T17:22:57Z

specutils/fitting/fitmodels.py

@@ -97,6 +97,7 @@ def _consecutive(data, stepsize=1):
    return np.split(data, np.where(np.diff(data) != stepsize)[0]+1)


+@warn_continuum_below_threshold(threshold=0.01, check=True)


I think you don't want check=True here (or below), right? Because then the do_continuum_function_check option gets ignored?

~~hah, good point~~ After we talked about it, we realized the check parameter doesn't even need to be there.

eteq

A few more things from a more detailed review. Mostly just docstring wordings but I also realized a couple of subtleties in the implementation.

specutils/analysis/flux.py

Co-Authored-By: Erik Tollerud <erik.tollerud@gmail.com>

eteq

I had a one-line addition but it was easier just to push up so I did that. Thanks @brechmos-stsci , this now all looks good!

created initial pass of check on continuum baseline

93f2094

brechmos-stsci force-pushed the i535-check-continuum-subtracted branch from bfd8fc8 to 93f2094 Compare October 16, 2019 15:33

brechmos added 2 commits October 16, 2019 13:54

first pass at decorator

6958184

Merge branch 'master' of github.com:astropy/specutils into i535-check…

b46f472

…-continuum-subtracted

eteq reviewed Oct 18, 2019

View reviewed changes

Craig Jones and others added 5 commits October 21, 2019 09:24

Update specutils/analysis/flux.py

cf28f74

Co-Authored-By: Erik Tollerud <erik.tollerud@gmail.com>

Merge branch 'i535-check-continuum-subtracted' of github.com:brechmos…

c8413fd

…-stsci/specutils into i535-check-continuum-subtracted

changed decorator, added test, changed to warning

9ddf3ef

fixed up decorator call

a3caeaf

removed print statements

418cdbd

chnaged to threshold, changed name of method

72446dc

brechmos-stsci marked this pull request as ready for review October 28, 2019 20:28

pep8

9d3469b

eteq requested changes Oct 28, 2019

View reviewed changes

brechmos added 3 commits October 29, 2019 09:15

conf name change, documentation added

addb7aa

re-worked https ref

99eea99

pep8

4b27367

eteq requested changes Oct 29, 2019

View reviewed changes

brechmos added 3 commits October 29, 2019 14:27

corrected check parameter

08f7152

udpated warning message

117a206

fixed spelling

5086be8

eteq requested changes Oct 30, 2019

View reviewed changes

Craig Jones and others added 6 commits October 30, 2019 14:53

Update specutils/analysis/flux.py

df89537

Co-Authored-By: Erik Tollerud <erik.tollerud@gmail.com>

Update specutils/analysis/flux.py

20ec46f

Co-Authored-By: Erik Tollerud <erik.tollerud@gmail.com>

Update specutils/analysis/flux.py

e460e7e

Co-Authored-By: Erik Tollerud <erik.tollerud@gmail.com>

Update specutils/analysis/flux.py

7252688

Co-Authored-By: Erik Tollerud <erik.tollerud@gmail.com>

pep8

add71d1

changes requested by erik

868e941

brechmos and others added 2 commits October 31, 2019 11:51

fixed up tests.

c87850f

add check of warning to test

5296508

eteq approved these changes Nov 1, 2019

View reviewed changes

eteq merged commit 6747d2a into astropy:master Nov 1, 2019

nmearl added this to the v0.7 milestone Nov 7, 2019

nmearl mentioned this pull request May 6, 2020

Add a check for whether the spectrum is continuum subtracted for centroid #290

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple utility continuum baseline checker #538

Simple utility continuum baseline checker #538

brechmos-stsci commented Oct 16, 2019

eteq left a comment

eteq Oct 18, 2019

brechmos-stsci Oct 21, 2019

eteq Oct 24, 2019

brechmos-stsci commented Oct 22, 2019

eteq left a comment

eteq Oct 28, 2019

brechmos-stsci Oct 29, 2019

eteq Oct 28, 2019

brechmos-stsci Oct 29, 2019

eteq Oct 28, 2019

brechmos-stsci Oct 29, 2019

eteq Oct 28, 2019

brechmos-stsci Oct 29, 2019

eteq commented Oct 28, 2019

eteq left a comment

eteq Oct 29, 2019

eteq Oct 29, 2019

brechmos-stsci Oct 29, 2019

eteq Oct 29, 2019

brechmos-stsci Oct 29, 2019 •

edited

Loading

eteq left a comment

eteq left a comment

		@@ -97,6 +97,7 @@ def _consecutive(data, stepsize=1):
		return np.split(data, np.where(np.diff(data) != stepsize)[0]+1)


		@warn_continuum_below_threshold(threshold=0.01, check=True)

Simple utility continuum baseline checker #538

Simple utility continuum baseline checker #538

Conversation

brechmos-stsci commented Oct 16, 2019

eteq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brechmos-stsci commented Oct 22, 2019

eteq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eteq commented Oct 28, 2019

eteq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brechmos-stsci Oct 29, 2019 • edited Loading

Choose a reason for hiding this comment

eteq left a comment

Choose a reason for hiding this comment

eteq left a comment

Choose a reason for hiding this comment

brechmos-stsci Oct 29, 2019 •

edited

Loading