lineup2() should work with low intensity peaks #920
Conversation
prjemian
commented
Feb 15, 2024
prjemian
commented
- close lineup2: problem for peaks of height 1 #919
apstools/plans/alignment.py
Outdated
| value = (stats.max_y - stats.min_y) / stats.sigma | ||
| return value > peak_factor | ||
| value = (stats.max_y - stats.min_y) / stats.stddev_y | ||
| return abs(value) > peak_factor | ||
| except ZeroDivisionError: # not enough samples | ||
| try: | ||
| value = abs(stats.max_y / stats.min_y) |
There was a problem hiding this comment.
Do we want to keep the format uniform across tries. Line 322 we return abs, line 336 we do the abs before return. Doesn't really matter in the grand scheme but I think it would be nice
There was a problem hiding this comment.
See commit 58d7230
| "peak, base, noise, center, sigma, xlo, xhi, npts, nscans, tol, outcome", | ||
| [ | ||
| [1e5, 1e6, 10, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.05, False], # signal but high background | ||
| [1e5, 0, 10, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.05, True], # model peak | ||
| [1e5, 1e4, 10, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.1, True], # high background, poor resolution | ||
| [1e5, 1e4, 10, 0.1, 0.2, -0.7, 0.5, 11, 2, 0.05, True], # not much better | ||
| [-1e5, -1e4, 1e-8, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.1, True], | ||
| [1e5, -10, 10, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.05, True], | ||
| [1e-5, 0, 1e-8, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.05, True], | ||
| [1e-5, 1e-7, 1e-8, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.05, True], | ||
| [-1e5, 0, 1e-8, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.05, True], | ||
| [0, 0, 1e-8, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.05, False], # only noise | ||
| [0, 1, 0.1, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.05, False], # bkg + noise | ||
| [0, 1, 100, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.05, False], # bkg + big noise | ||
| [0, 0, 0, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.005, None], # no signal ZeroDivisionError | ||
| [0, 1, 0, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.005, None], # bkg only ZeroDivisionError |
There was a problem hiding this comment.
I don't know if we ever reuse these values however I think it might be useful/cleaner to store each list in a named variable as opposed to having such a list of lists. Hard to look at and understand what we are seeing, especially if we will be modifying test values.
Ex:
signal_high_background = [1e5, 1e6, 10, 0.1, 0.2, -0.7, 0.5, 11, 1, 0.05, False]
There was a problem hiding this comment.
If these are defined as individual lists before the test, it seems easy to dissociate the values with the name. Could a namedtuple be used? If so, the fields of the namedtuple could be the first argument, then a list of the namedtuples would be the second.
There was a problem hiding this comment.
One more commit coming ...
There was a problem hiding this comment.
I like this approach (251bc1e). It forces the person writing the test parameters to explain, in some way, why that set of parameters is used. The namedtuple and the type annotation is the Pythonic way to enforce that the same names are used for all parametrized tests.
There was a problem hiding this comment.
We can make that SOP going forward, and backfill the old tests by best efforts.
