Add flat field method to Observatory - WIP #729
Conversation
should probably be moved elsewhere but just moving for now to clean up
Codecov Report
@@ Coverage Diff @@
## develop #729 +/- ##
===========================================
+ Coverage 80.58% 80.65% +0.06%
===========================================
Files 61 61
Lines 5188 5211 +23
Branches 715 717 +2
===========================================
+ Hits 4181 4203 +22
- Misses 818 819 +1
Partials 189 189
Continue to review full report at Codecov.
|
| @@ -611,6 +618,156 @@ def close_dome(self): | |||
| self.logger.info('Closed dome') | |||
| return self.dome.close() | |||
|
|
|||
| def take_flat_fields(self, | |||
There was a problem hiding this comment.
This method feels like it has too many parameters and responsibilities. I'd love to see it shorter, and composed from more (ideally) reusable methods.
There was a problem hiding this comment.
Absolutely agree. Might happen here and might wait for future PR.
| time to each flat-field image. | ||
| Args: | ||
| which (str, optional): Specify either 'evening' or 'morning' to lookup coordinates | ||
| in config, default 'evening'. |
There was a problem hiding this comment.
Why not just require alt and az be passed in, leave the caller with the responsibility to deal with time of day.
There was a problem hiding this comment.
We also need to know if it is getting darker or brighter, i.e. is evening or morning, so that we can adjust the exposure time properly.
| # Check the counts for each image | ||
| is_saturated = False | ||
| for cam_name, info in camera_events.items(): | ||
| img_file = info['filename'].replace('.cr2', '.fits') |
There was a problem hiding this comment.
Lots of this loop looks like it could be extracted into helper methods.
| @@ -379,3 +379,18 @@ def test_operate_dome(config_with_simulated_dome): | |||
| assert observatory.open_dome() | |||
| assert observatory.dome.is_open | |||
| assert not observatory.dome.is_closed | |||
|
|
|||
|
|
|||
| def test_create_flat_field(observatory): | |||
There was a problem hiding this comment.
This implies that most of the new code is untested (i.e. no automated test, not that you haven't tested it on PAN001).
There was a problem hiding this comment.
Yes. I had not marked this for review yet because I haven't written the tests. I just wrote some to cover the creation of the flat-field Observation but haven't yet figured out a good way to do the actual take_flat_field method. So far this PR is just extracting from #541. I'll mark as WIP until I think it is ready but as mentioned I might just end up with an alternative PR.
and is working and we want to be able to use it on sky sooner than we can probably work out the testing. Issue panoptes#732 includes testing as part of its requirement.
|
@jamessynge @AnthonyHorton I am going to merge this tomorrow without any test coverage (there is a Let me know if there are any serious concerns with merging without tests. Note that the flat fielding is not yet integrated into the machine so this will not change any current behaviour. |
This will pass all `key=value` options directly to `Observatory.take_flat_fields`. Dependent on panoptes#729
on something that is hard-coded.
Better docs.
|
@jamessynge @AnthonyHorton I'm considering moving some of this logic up to the However it feels a bit odd to move it up to that level. Ideally it would just be abstracted into a mini separate state-machine loop. |
wtgee
changed the title
* This method has been used off and on variously for a while. Ideally should be decoupled more from the hardware but getting it committed as is. Replaces and closes panoptes#729.
* This method has been used off and on variously for a while. Ideally should be decoupled more from the hardware but getting it committed as is. Replaces and closes #729.
This is the actual method that takes the flat fields.
Edit: The flat-field overview has been moved to #732
This is pulled from #541.