Merge changes from May Huntsman commissioning run #535
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #535 +/- ##
===========================================
- Coverage 70.54% 69.79% -0.75%
===========================================
Files 64 65 +1
Lines 5470 5841 +371
Branches 760 841 +81
===========================================
+ Hits 3859 4077 +218
- Misses 1403 1547 +144
- Partials 208 217 +9
Continue to review full report at Codecov.
|
|
Thanks @AnthonyHorton. A lot of these changes are in #511. I'll do an actual review in a bit as this PR might be better than that one since it is fresher. |
| @@ -14,7 +14,8 @@ environment: | |||
| weather: | |||
| station: mongo | |||
| aag_cloud: | |||
| serial_port: '/dev/ttyUSB1' | |||
| # serial_port: '/dev/ttyUSB1' | |||
| serial_port: '/dev/tty.USA19H2P1.1' | |||
There was a problem hiding this comment.
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.
pocs/core.py
Outdated
| @@ -181,6 +182,13 @@ def say(self, msg): | |||
| self.logger.info('Unit says: {}', msg) | |||
| self.send_message(msg, channel='PANCHAT') | |||
|
|
|||
| if 'slack_webhook_url' in self.config: | |||
There was a problem hiding this comment.
Some of the slack and twitter stuff got officially pulled into POCS, so let's strip all this from here.
There was a problem hiding this comment.
OK, removed.
| # 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.
Any chance you'll want to change the degree of this, i.e. pull from config?
There was a problem hiding this comment.
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.
pocs/observatory.py
Outdated
| @@ -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.
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.
I think it's doing something fairly sensible. self._coarse_focus_done is set to False in the constructor, so unless coarse has been set in the call to autofocus_cameras then coarse will get set to True here, and so it will do a coarse focus the first time autofocus_cameras is called.
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.
Sadly I think I wrote this code so I would hopefully understand it. Actually, I missed the not in the logic. Thanks.
pocs/observatory.py
Outdated
| @@ -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.
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.
It is all a bit odd. It sets self._coarse_focus_done to True after any call to autofocus_cameras, regardless of whether a coarse focus was actually performed.
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.
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.
OK. Will remove it for now and see how we go during the next Huntsman run, and reinstate if needed.
pocs/utils/theskyx.py
Outdated
| @@ -56,6 +56,7 @@ def read(self, timeout=5): | |||
|
|
|||
| try: | |||
| response = self.socket.recv(4096).decode() | |||
| self.logger.debug('Response from reading TSX socket: {}'.format(response)) | |||
There was a problem hiding this comment.
This should be removed. I was using it for debug and it generated way too much output.
|
@AnthonyHorton , if you see this in time could you actually split this into separate PRs? After the removal of the items commented on above there is essentially:
These are all separate things, so that will keep it a little cleaner. I'm guilty of the same thing on #511 but @jamessynge was good and called me out on it. ;) |
pocs/core.py
Outdated
| @@ -5,6 +5,7 @@ | |||
| import warnings | |||
| import multiprocessing | |||
| import zmq | |||
| import requests | |||
There was a problem hiding this comment.
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.
OK, removing for now.
A new Huntsman commissioning run (and a bunch of frenzied coding) is imminent, so I'd like to merge the assorted minor changes from the previous run. These are a few bug fixes, plus some more substantial changes to the autofocus routine and the Bisque dome control.