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

Remove spline smoothing focus #621

Merged
merged 13 commits into from
Sep 24, 2018

Conversation

wtgee
Copy link
Member

@wtgee wtgee commented Sep 22, 2018

More things pulled out of #547. I'm not entirely sure these should all be separate but it will but they don't seem to be Pyro4 specific.

* Take darks after the coarse focus
@wtgee wtgee requested a review from AnthonyHorton September 22, 2018 04:10
@wtgee wtgee added the Camera label Sep 22, 2018
* Fix some dark_thumb changes
* Use os.path.join
@wtgee wtgee requested a review from jamessynge September 22, 2018 09:31
Copy link
Contributor

@jamessynge jamessynge left a comment

Choose a reason for hiding this comment

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

It looks OK on the surface, but I'm not familiar with this code.

@@ -332,7 +302,8 @@ def autofocus(self,
'plots': plots,
'start_event': None,
'finished_event': coarse_event,
**kwargs})
**kwargs}
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, start_event and finished_event are not params to autofocus(), so it is possible for them to be in **kwargs, which would cause a runtime exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the kwargs here is also showing as some kind of linting error in my editor although it doesn't give more information. I'll clean this up a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, there is nothing in the underlying _autofocus that is using either args or kwargs. I'm going to do some more cleanup.

pocs/focuser/focuser.py Show resolved Hide resolved
pocs/focuser/focuser.py Show resolved Hide resolved
…geable.

Would like to split out the plotting and fitting into separate helper
methods but am going to leave for now.

* All plotting moved to one location.
* Figure out `focus_type` string so no `if` logic for logging output.
* More cleanup of paths.
@wtgee
Copy link
Member Author

wtgee commented Sep 22, 2018

I was originally just moving things over from #574 but now I've started an actual cleanup. For better or for worse. 😬

@codecov
Copy link

codecov bot commented Sep 23, 2018

Codecov Report

Merging #621 into develop will increase coverage by <.01%.
The diff coverage is 83.6%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #621      +/-   ##
===========================================
+ Coverage    69.26%   69.27%   +<.01%     
===========================================
  Files           65       65              
  Lines         5698     5696       -2     
  Branches       807      799       -8     
===========================================
- Hits          3947     3946       -1     
- Misses        1533     1535       +2     
+ Partials       218      215       -3
Impacted Files Coverage Δ
pocs/camera/camera.py 90% <ø> (-1.06%) ⬇️
pocs/focuser/focuser.py 76.92% <83.6%> (+1.01%) ⬆️

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 5938d97...705ab69. Read the comment docs.

* Pull out shared parameters from focus
* Rearranged underlying _autofocus params to reflect how focus_parmas is
set up (which is a more logical order - doesn't really matter in the end).
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.

Lots of changes! Looks good though.

Edit: I mean, apart from the fact it's now failing tests ;-)

'merit_function': merit_function,
'merit_function_kwargs': merit_function_kwargs,
'mask_dilations': mask_dilations,
'spline_smoothing': spline_smoothing,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need any of the spline_smoothing arguments any more. They're no longer used.

@wtgee
Copy link
Member Author

wtgee commented Sep 24, 2018

Edit: I mean, apart from the fact it's now failing tests ;-)

Unfortunately the test_autofocus_all test is just hanging half of the time. Usually restarting the test fixes it. I'm going to merge in some of the other cleanup and see if it still happens.

@wtgee wtgee merged commit 9580fa9 into panoptes:develop Sep 24, 2018
@wtgee wtgee deleted the remove-spline-smoothing-focus branch September 24, 2018 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants