Skip to content
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.

Sign up for GitHub

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

Observatory Dependency Injection #195

Merged
merged 3 commits into from
Dec 16, 2017
Merged

Observatory Dependency Injection #195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c006be
Dependency Injection
wtgee Dec 7, 2017
03c9766
Merge branch 'develop' of https://github.com/panoptes/POCS into obser…
wtgee Dec 15, 2017
678ca33
Adjust pocs_shell to create observatory
wtgee Dec 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion bin/pocs_shell
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ from astropy.io import fits
from astropy.utils import console

from pocs import POCS
from pocs.observatory import Observatory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please import package or module, not class. In this case, you'd say:
import pocs.observatory
And later:
observatory = pocs.observatory.Observatory(simulator=simulator)

Copy link
Member Author

@wtgee wtgee Dec 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I remarked in another PR, this was not something that we explicitly decided on, we only talked about using root relative syntax. Either style is correct and the thing that is usually stressed is to be consistent. Since 95% of the current code uses the from syntax, which is what we have been encouraging previously, I'm not sure what the arguments are for in favor of changing them all. It is slightly more obvious what package is being referred to so I could understand wanting to switch to it, but that is a conversation we have not had yet.

Copy link
Member Author

@wtgee wtgee Dec 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you made a comment but disappeared. I looked at the google style code a bit more and see what they are getting at. It's not explicitly saying not to use the from style but to use just the module without prefix, which is not what I am doing. Will fix accordingly. From here

from pocs.scheduler.field import Field
from pocs.scheduler.observation import Observation
from pocs.utils import current_time
Expand Down Expand Up @@ -125,7 +126,8 @@ class PocsShell(Cmd):
simulator = []

try:
self.pocs = POCS(simulator=simulator, messaging=True)
observatory = Observatory(simulator=simulator)
self.pocs = POCS(observatory, messaging=True)
self.pocs.initialize()
except error.PanError:
pass
Expand Down
11 changes: 8 additions & 3 deletions pocs/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from astropy import units as u

from . import PanBase
from .observatory import Observatory
from .state.machine import PanStateMachine
from .utils import current_time
from .utils import get_free_space
Expand All @@ -25,6 +24,7 @@ class POCS(PanStateMachine, PanBase):
the `get_ready()` method the transition that is responsible for moving to the initial state.

Args:
observatory(Observatory): An instance of a `pocs.observatory.Observatory` class
state_machine_file(str): Filename of the state machine to use, defaults to
'simple_state_table'
messaging(bool): If messaging should be included, defaults to False
Expand All @@ -38,7 +38,12 @@ class POCS(PanStateMachine, PanBase):

"""

def __init__(self, state_machine_file='simple_state_table', messaging=False, **kwargs):
def __init__(
self,
observatory,
state_machine_file='simple_state_table',
messaging=False,
**kwargs):

# Explicitly call the base classes in the order we want
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this comment and the associated code seem to imply that we should use aggregation for PanStateMachine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be the comment that you missed, about the awkwardness of multiple inheritance.

PanBase.__init__(self, **kwargs)
Expand All @@ -61,7 +66,7 @@ def __init__(self, state_machine_file='simple_state_table', messaging=False, **k
PanStateMachine.__init__(self, state_machine_file, **kwargs)

# Create our observatory, which does the bulk of the work
self.observatory = Observatory(**kwargs)
self.observatory = observatory

self._connected = True
self._initialized = False
Expand Down
28 changes: 20 additions & 8 deletions pocs/tests/test_pocs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,24 @@
from pocs import POCS
from pocs import _check_config
from pocs import _check_environment
from pocs.observatory import Observatory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto on the import style.

from pocs.utils import error
from pocs.utils.messaging import PanMessaging


@pytest.fixture
def pocs(config):
def observatory():
observatory = Observatory(simulator=['all'])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop blank line.

yield observatory


@pytest.fixture
def pocs(config, observatory):
os.environ['POCSTIME'] = '2016-08-13 13:00:00'
pocs = POCS(simulator=['all'], run_once=True,

pocs = POCS(observatory,
run_once=True,
config=config,
ignore_local_config=True, db='panoptes_testing')

Expand Down Expand Up @@ -177,11 +187,13 @@ def test_is_weather_safe_no_simulator(pocs, db):
assert pocs.is_weather_safe() is False


def test_run_wait_until_safe(db):
def test_run_wait_until_safe(db, observatory):
os.environ['POCSTIME'] = '2016-08-13 23:00:00'

def start_pocs():
pocs = POCS(simulator=['camera', 'mount', 'night'],
observatory.config['simulator'] = ['camera', 'mount', 'night']

pocs = POCS(observatory,
messaging=True, safe_delay=15)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, looks like safe_delay is one of several undocumented parameters to POCS.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created issue #198

pocs.db.current.remove({})
pocs.initialize()
Expand Down Expand Up @@ -286,9 +298,9 @@ def test_run(pocs):
assert pocs.state == 'sleeping'


def test_run_interrupt_with_reschedule_of_target():
def test_run_interrupt_with_reschedule_of_target(observatory):
def start_pocs():
pocs = POCS(simulator=['all'], messaging=True)
pocs = POCS(observatory, messaging=True)
pocs.logger.info('Before initialize')
pocs.initialize()
pocs.logger.info('POCS initialized, back in test')
Expand Down Expand Up @@ -321,9 +333,9 @@ def start_pocs():
assert pocs_process.is_alive() is False


def test_run_power_down_interrupt():
def test_run_power_down_interrupt(observatory):
def start_pocs():
pocs = POCS(simulator=['all'], messaging=True)
pocs = POCS(observatory, messaging=True)
pocs.initialize()
pocs.observatory.scheduler.fields_list = [{'name': 'KIC 8462852',
'position': '20h06m15.4536s +44d27m24.75s',
Expand Down