-
Notifications
You must be signed in to change notification settings - Fork 49
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
Merge changes from May Huntsman commissioning run #535
Changes from 8 commits
49ad768
e364a6b
320865e
191cf27
2d4c83d
3553b9e
eec7a12
fe92b92
bdad372
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 |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
import warnings | ||
import multiprocessing | ||
import zmq | ||
import requests | ||
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. Not needed, was part of the slack stuff. Will pull through with a merge from upstream/develop anyway. 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. OK, removing for now. |
||
|
||
from astropy import units as u | ||
|
||
|
@@ -121,7 +122,6 @@ def has_messaging(self, value): | |
def should_retry(self): | ||
return self._obs_run_retries >= 0 | ||
|
||
|
||
################################################################################################## | ||
# Methods | ||
################################################################################################## | ||
|
@@ -396,7 +396,6 @@ def has_free_space(self, required_space=0.25 * u.gigabyte): | |
free_space = get_free_space() | ||
return free_space.value >= required_space.to(u.gigabyte).value | ||
|
||
|
||
################################################################################################## | ||
# Convenience Methods | ||
################################################################################################## | ||
|
@@ -442,7 +441,6 @@ def wait_until_safe(self): | |
while not self.is_safe(no_warning=True): | ||
self.sleep(delay=self._safe_delay) | ||
|
||
|
||
################################################################################################## | ||
# Class Methods | ||
################################################################################################## | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import matplotlib.colors as colours | ||
import matplotlib.pyplot as plt | ||
|
||
from scipy.interpolate import UnivariateSpline | ||
from astropy.modeling import models, fitting | ||
from scipy.ndimage import binary_dilation | ||
|
||
import numpy as np | ||
|
@@ -437,22 +437,38 @@ def _autofocus(self, | |
best_focus = focus_positions[imax] | ||
|
||
elif not coarse: | ||
# Crude guess at a standard deviation for focus metric, 40% of the maximum value | ||
weights = np.ones(len(focus_positions)) / (spline_smoothing * metric.max()) | ||
|
||
# Fit smoothing spline to focus metric data | ||
fit = UnivariateSpline(focus_positions, metric, w=weights, k=4, ext='raise') | ||
|
||
try: | ||
stationary_points = fit.derivative().roots() | ||
except ValueError as err: | ||
self.logger.warning('Error finding extrema of spline fit: {}'.format(err)) | ||
best_focus = focus_positions[imax] | ||
else: | ||
extrema = fit(stationary_points) | ||
if len(extrema) > 0: | ||
best_focus = stationary_points[extrema.argmax()] | ||
fitted = True | ||
# Fit data around the maximum value to determine best focus position. | ||
# Initialise models | ||
shift = models.Shift(offset=-focus_positions[imax]) | ||
poly = models.Polynomial1D(degree=4, c0=1, c1=0, c2=-1e-2, c3=0, c4=-1e-4, | ||
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. Any chance you'll want to change the degree of this, i.e. pull from config? 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. It's possible, but unlikely I think. As well as changing the degree of the polynomial you'd need to change the number of points around the maximum used for the fitting, so it isn't an under or over constrained fitting problem. |
||
fixed={'c0': True, 'c1': True, 'c3': True}) | ||
scale = models.Scale(factor=metric[imax]) | ||
reparameterised_polynomial = shift | poly | scale | ||
|
||
# Initialise fitter | ||
fitter = fitting.LevMarLSQFitter() | ||
|
||
# Select data range for fitting. Tries to use 2 points either side of max, if in range. | ||
fitting_indices = (max(imax - 2, 0), min(imax + 2, n_positions - 1)) | ||
|
||
# Fit models to data | ||
fit = fitter(reparameterised_polynomial, | ||
focus_positions[fitting_indices[0]:fitting_indices[1] + 1], | ||
metric[fitting_indices[0]:fitting_indices[1] + 1]) | ||
|
||
best_focus = -fit.offset_0 | ||
fitted = True | ||
|
||
# Guard against fitting failures, force best focus to stay within sweep range | ||
if best_focus < focus_positions[0]: | ||
self.logger.warning("Fitting failure: best focus {} below sweep limit {}".format(best_focus, | ||
focus_positions[0])) | ||
best_focus = focus_positions[1] | ||
|
||
if best_focus > focus_positions[-1]: | ||
self.logger.warning("Fitting failure: best focus {} above sweep limit {}".format(best_focus, | ||
focus_positions[-1])) | ||
best_focus = focus_positions[-2] | ||
|
||
else: | ||
# Coarse focus, just use max value. | ||
|
@@ -462,8 +478,9 @@ def _autofocus(self, | |
ax2 = fig.add_subplot(3, 1, 2) | ||
ax2.plot(focus_positions, metric, 'bo', label='{}'.format(merit_function)) | ||
if fitted: | ||
fs = np.arange(focus_positions[0], focus_positions[-1] + 1) | ||
ax2.plot(fs, fit(fs), 'b-', label='Smoothing spline fit') | ||
fs = np.arange(focus_positions[fitting_indices[0]], | ||
focus_positions[fitting_indices[1]] + 1) | ||
ax2.plot(fs, fit(fs), 'b-', label='Polynomial fit') | ||
|
||
ax2.set_xlim(focus_positions[0] - focus_step / 2, focus_positions[-1] + focus_step / 2) | ||
u_limit = 1.10 * metric.max() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,7 @@ def __init__(self, *args, **kwargs): | |
self.cameras = OrderedDict() | ||
self._primary_camera = None | ||
self._create_cameras(**kwargs) | ||
self._coarse_focus_done = False | ||
|
||
# TODO(jamessynge): Discuss with Wilfred the serial port validation behavior | ||
# here compared to that for the mount. | ||
|
@@ -455,7 +456,7 @@ def get_standard_headers(self, observation=None): | |
|
||
return headers | ||
|
||
def autofocus_cameras(self, camera_list=None, coarse=False): | ||
def autofocus_cameras(self, camera_list=None, coarse=None): | ||
""" | ||
Perform autofocus on all cameras with focus capability, or a named subset | ||
of these. Optionally will perform a coarse autofocus first, otherwise will | ||
|
@@ -485,6 +486,9 @@ def autofocus_cameras(self, camera_list=None, coarse=False): | |
|
||
autofocus_events = dict() | ||
|
||
if coarse is None: | ||
coarse = not self._coarse_focus_done | ||
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. Seems like either this variable name is wrong or the default logic is wrong. Doesn't this end up doing a coarse focus every time but the first time? Also see setter below. 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. I think it's doing something fairly sensible. Not clear to me that we actually want it to do this, though. Coarse focus is only needed in a pretty limited set of circumstances already, and will only be needed less with time. 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. Sadly I think I wrote this code so I would hopefully understand it. Actually, I missed the |
||
|
||
# Start autofocus with each camera | ||
for cam_name, camera in cameras.items(): | ||
self.logger.debug("Autofocusing camera: {}".format(cam_name)) | ||
|
@@ -507,6 +511,8 @@ def autofocus_cameras(self, camera_list=None, coarse=False): | |
else: | ||
autofocus_events[cam_name] = autofocus_event | ||
|
||
self._coarse_focus_done = True | ||
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. Along with above, seems like this should check if a coarse focus was done and if so set variable. It is currently always set to True as far as I can tell. 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. It is all a bit odd. It sets I wondering whether we should just remove all of this. We hardly every want to actually do a coarse focus, and I feel like any logic for deciding whether to do it or not belongs more in the state machine than here. 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. That's okay with me so I leave the decision up to you. We could feasibly build a coarse-focus state that we could transition to at various points. 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. OK. Will remove it for now and see how we go during the next Huntsman run, and reinstate if needed. |
||
|
||
return autofocus_events | ||
|
||
def open_dome(self): | ||
|
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.
We should probably just comment this out and force the use of
peas_local.yaml
. I've been thinking of doing that system wide to make it obvious to new installers what needs to be changed in the config. Should be okay for here, just mostly thinking out loud.