More permissive masking for kinematic constraints #267
Conversation
|
@mducle @davidvoneshen This PR is "draft" because it lacks testing/docs, but I'd appreciate input on the principle of the thing ;-) |
|
Here is the script to produce the above plot, for anyone interested in playing around. You need to point it at a Spectrum2D .json file; you can make these with #! /usr/bin/env python3
# This requires a build of Euphonic in which the "strict" argument is available
# for euphonic.spectra.apply_kinematic_constraints, e.g. d068b5c
from argparse import ArgumentParser
from pathlib import Path
from euphonic import ureg, Spectrum2D
from euphonic.plot import plot_2d_to_axis
from euphonic.spectra import apply_kinematic_constraints
import matplotlib.pyplot as plt
def get_parser() -> ArgumentParser:
parser = ArgumentParser(
description='Demonstrate different masking rules for 2-D data in Euphonic')
parser.add_argument('spectrum', type=Path, help='Euphonic Spectrum2D data file')
parser.add_argument('--angle-range', nargs=2, default=(134.5, 135.5),
dest='angle_range', type=float)
return parser
def main():
args = get_parser().parse_args()
spectrum = Spectrum2D.from_json_file(args.spectrum)
fig, axes = plt.subplots(ncols=2)
for ax, strict in zip(axes, [True, False]):
masked_spectrum = apply_kinematic_constraints(
spectrum,
angle_range=args.angle_range,
e_f = 32 * ureg('1/cm'),
strict=strict)
plot_2d_to_axis(masked_spectrum, ax)
ax.set_title('Existing' if strict else 'Relaxed')
fig.suptitle(f'Angle range: {args.angle_range[0]}°-{args.angle_range[1]}°')
plt.show()
if __name__ == '__main__':
main() |
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more. @@ Coverage Diff @@
## master #267 +/- ##
=========================================
Coverage ? 95.78%
=========================================
Files ? 28
Lines ? 3958
Branches ? 798
=========================================
Hits ? 3791
Misses ? 98
Partials ? 69
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Given that no strong opinions have been expressed I will make executive decision to consider the existing behaviour a bug; in this case we can lose some extra logic here. The PR remains draft because of its dependence on #245 ; when that is merged this can be polished and opened for review. |
|
Hi Adam, terribly sorry I forgot to look at this. Having now done so I agree. |
ajjackson
force-pushed
the
permissive-masking
branch
from
d068b5c to
09ba918
Compare
Instead of requiring that bins lie entirely in accessible energy range, require that any part of bin is in range. This avoids creating a jagged line with gaps when using a very narrow angle range (e.g. for a single detector bank.)
ajjackson
force-pushed
the
permissive-masking
branch
from
09ba918 to
3a50164
Compare
|
Updating test data: as we can see the underlying data looks the same, but the new outputs mask away slightly less of it
|
A few NaN values become other values as the masking approach has been slightly relaxed.
|
Updated the script test data: as expected the only changes are that some NaN become values. |
Currently the masking logic for kinematic constrains includes only energy bins that fall entirely within the accessible energy range at the Q-bin mid-point. For narrow angle ranges, this can lead to rather ugly/inconvenient gaps in the spectrum -- and for the special case of an exact angle, no bins are included at all.
Here I have implemented a more permissive logic that unmasks the bin if any part of the energy range is accessible at this Q-bin midpoint.
At the moment of opening this PR it is implemented with an optional argument, so the existing behaviour remains default. However, I am inclined to declare the existing behaviour "a bug" and drop it; I can't see many situations where it would be preferable and you can always reduce the bin widths for more accuracy.
This new behaviour is required in order to perform numerical simulations of 1-D instruments such as TOSCA including dispersion. (The existing method in AbINS makes a DOS-like approximation).