Skip to content

Timer Utility Change#625

Merged
wtgee merged 10 commits intopanoptes:developfrom
wtgee:generic-wait-events-function
Sep 24, 2018
Merged

Timer Utility Change#625
wtgee merged 10 commits intopanoptes:developfrom
wtgee:generic-wait-events-function

Conversation

@wtgee
Copy link
Copy Markdown
Member

@wtgee wtgee commented Sep 23, 2018

  • Timeout renamed to Timer to reflect usage. Note, I did not rename the serialutil equivalent.
  • Generic helper method in POCS to wait for events, essentially a wrapper around the Timer.
  • Cleanup of observing and pointing states to use wait_for_events

* `Timeout` renamed to `Timer` to reflect usage. Note, I did not rename
the serialutil equivalent.
* Generic helper method in POCS to wait for events, essentially a wrapper
around the `Timer`.
* Cleanup of `observing` and `pointing` states to use `wait_for_events`
@wtgee wtgee requested a review from jamessynge September 23, 2018 00:20
Comment thread pocs/core.py Outdated
"""Wait for event(s) to be set.

This method will wait for a maximum of `timeout` seconds for all of the
`events` to complete. The loop will `check_messages` each time in case of
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will check at least every sleep_delay seconds for the events to be done, and also for interrupts and bad weather. Will log debug messages approximately every status_interval seconds, and will output status messages approximately every status_interval seconds.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, I think I was getting lazy. :)

Comment thread pocs/core.py Outdated
raise error.Timeout

# Sleep for a little bit.
time.sleep(sleep_delay.value)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The .value makes me think you're passing in an astropy time quantity rather than the documented float.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Changed to support both. It was somewhere in between.

Comment thread pocs/core.py
break

now = current_time()
if now >= next_msg_time:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW, you could choose to use Timer for this too, but that isn't trivial because it isn't a PeriodicTimer. Perhaps we should write such a class... sometime.

MAX_EXTRA_TIME = 60 # seconds


def on_enter(event_data):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can probably streamline the docstring now.

if wait_time > timeout:
raise error.Timeout("Timeout waiting for pointing image")
# Start the exposure
camera_event = primary_camera.take_observation(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the type of camera_event list of events? If so, please rename as camera_events, observation_done_events, or some such.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Here it's just a single event. The wait_for_events handles either a single Event or a list.

Comment thread pocs/utils/__init__.py Outdated

# This is a streamlined variant of PySerial's serialutil.Timeout.
class Timeout(object):
class Timer(object):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suppose this is a good change since I see we already have an error type called Timeout. However, note also that we're using threading.Timer in some places. If you are concerned by that, you could consider calling this TimeLimit, which is not a symbol that we use.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

CountdownTimer?

@jamessynge
Copy link
Copy Markdown
Contributor

I forgot to say, can you come up with any kind of focused test?

* Add basic tests
* Update docstrings
* Handle Quantity for timeout and sleep_delay better
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 23, 2018

Codecov Report

❗ No coverage uploaded for pull request base (develop@9580fa9). Click here to learn what that means.
The diff coverage is 91.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #625   +/-   ##
==========================================
  Coverage           ?   67.92%           
==========================================
  Files              ?       65           
  Lines              ?     5690           
  Branches           ?      798           
==========================================
  Hits               ?     3865           
  Misses             ?     1615           
  Partials           ?      210
Impacted Files Coverage Δ
pocs/state/states/default/observing.py 20% <0%> (ø)
pocs/core.py 79.79% <100%> (ø)
pocs/utils/__init__.py 80% <100%> (ø)
pocs/state/states/default/pointing.py 44.23% <85.71%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9580fa9...05e3ef0. Read the comment docs.

* Adding an `overwrite` param to `write_fits`.
* Testing images have same filename, so overwrite each other.
* Cleanup of some astropy units
@wtgee
Copy link
Copy Markdown
Member Author

wtgee commented Sep 23, 2018

Note that test coverage should increase after #627.

@wtgee wtgee merged commit 9d0240f into panoptes:develop Sep 24, 2018
@wtgee wtgee deleted the generic-wait-events-function branch September 24, 2018 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants