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.

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

Observation current_exp change #668

Merged
merged 29 commits into from
Oct 10, 2018
Merged

Conversation

wtgee
Copy link
Member

@wtgee wtgee commented Oct 3, 2018

Changed to current_exp_num to be more consistent.

This is now just a count of the exposure_list, which means we don't need to do specific bookkeeping.

Adds a change to actually reset exposure_list when reset is called.

This is now just a count of the `exposure_list`, which means we don't need to do specific bookkeeping.

Adds a change to actually reset `exposure_list` when `reset` is called.
@wtgee wtgee added the Quick PR This PR should be quick to review. label Oct 3, 2018
Copy link
Collaborator

@AnthonyHorton AnthonyHorton left a comment

Choose a reason for hiding this comment

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

Look like good changes, but tests seem to be consistently hanging in test_run_complete (I ran them again to check).

@wtgee
Copy link
Member Author

wtgee commented Oct 4, 2018

One of those frustrating blocks on test_run_complete. Clearly something is going wrong in the testing but am unable to reproduce locally.

@wtgee
Copy link
Member Author

wtgee commented Oct 4, 2018

This is hanging every time in the test_run_complete, but only when done on travis. I'm tempted to close as there is no clear need for this change but I would like to discover why it is failing. Leaving open for a few days.

@@ -157,10 +164,10 @@ def last_exposure(self):
##################################################################################################

def reset(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if this method were called by __init__, so that they can't get out of sync.

@@ -127,6 +125,15 @@ def directory(self):
self.field.field_name)
return self._directory

@property
def current_exp_num(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we're currently taking another exposure? Is that counted or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

This value will go up inside the camera.take_observation method but before the call to take_exposure. So yes, the current_exp_num will reflect an image in the process of being taken.

pocs/scheduler/observation.py Show resolved Hide resolved
@@ -27,7 +27,6 @@ def on_enter(event_data):
pocs.logger.warning("Problem with imaging: {}".format(e))
pocs.say("Hmm, I'm not sure what happened with that exposure.")
else:
pocs.observatory.current_observation.current_exp += 1
pocs.logger.debug('Finished with observing, going to analyze')

Copy link
Contributor

Choose a reason for hiding this comment

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

Drop the blank line?

@wtgee
Copy link
Member Author

wtgee commented Oct 5, 2018

This is hanging every time in the test_run_complete, but only when done on travis. I'm tempted to close as there is no clear need for this change but I would like to discover why it is failing. Leaving open for a few days.

The hang here is coming from the camera simulator trying to use the same image each time. On my local I now get:

D1005 04:26:48.701 Taking 2.0 second exposure on Cam01: /var/panoptes//images/fields/Kic8462852/SC0910/20160909T080000/20160909T080000.fits              
E1005 04:26:50.721 Error writing image to /var/panoptes//images/fields/Kic8462852/SC0550/20160909T080000/20160909T080000.fits!
E1005 04:26:50.721 Error writing image to /var/panoptes//images/fields/Kic8462852/SC0910/20160909T080000/20160909T080000.fits!
E1005 04:26:50.721 File '/var/panoptes//images/fields/Kic8462852/SC0550/20160909T080000/20160909T080000.fits' already exists.
E1005 04:26:50.721 File '/var/panoptes//images/fields/Kic8462852/SC0910/20160909T080000/20160909T080000.fits' already exists.

and it loops forever. Will work on getting a fix for it. Will be somewhat related to other camera tests.

@jamessynge
Copy link
Contributor

Regarding test_run_complete, I can't help but note the // (double slash) in the paths. Fixing that probably won't help... but might!

@wtgee
Copy link
Member Author

wtgee commented Oct 5, 2018

Regarding test_run_complete, I can't help but note the // (double slash) in the paths. Fixing that probably won't help... but might!

Both you and @AnthonyHorton pointed it out so I would be a fool to ignore fixing it! Edit: #680

@wtgee
Copy link
Member Author

wtgee commented Oct 6, 2018

These tests will clear after #681 and #684 are merged.

@wtgee wtgee removed the Quick PR This PR should be quick to review. label Oct 6, 2018
@codecov
Copy link

codecov bot commented Oct 10, 2018

Codecov Report

Merging #668 into develop will decrease coverage by 0.22%.
The diff coverage is 96.29%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #668      +/-   ##
===========================================
- Coverage    77.92%   77.69%   -0.23%     
===========================================
  Files           61       61              
  Lines         5069     5080      +11     
  Branches       695      697       +2     
===========================================
- Hits          3950     3947       -3     
- Misses         910      921      +11     
- Partials       209      212       +3
Impacted Files Coverage Δ
pocs/state/states/default/observing.py 72.22% <ø> (-1.47%) ⬇️
pocs/camera/simulator.py 98.21% <ø> (-0.04%) ⬇️
pocs/observatory.py 89.49% <100%> (-0.29%) ⬇️
pocs/camera/camera.py 91.14% <100%> (+1.84%) ⬆️
pocs/state/states/default/pointing.py 64.7% <100%> (ø) ⬆️
pocs/scheduler/observation.py 94.73% <100%> (+0.53%) ⬆️
pocs/state/states/default/analyzing.py 56.25% <66.66%> (ø) ⬆️
pocs/utils/images/__init__.py 60.3% <0%> (-5.53%) ⬇️
pocs/utils/images/fits.py 81.31% <0%> (-2.03%) ⬇️
pocs/dome/astrohaven.py 71.05% <0%> (-1.76%) ⬇️
... and 3 more

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 c31266a...cba763d. Read the comment docs.

@wtgee wtgee merged commit 268110a into panoptes:develop Oct 10, 2018
@wtgee wtgee deleted the current_exp-from-list branch October 10, 2018 10:01
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.

3 participants