Basic horizon constraint features #368

wtgee · 2018-01-16T12:48:14Z

This is an attempt to get basic support for a custom horizon into the codebase. This is neither feature complete nor robust but hopefully is enough of a stepping stone such that others could work on the code.

Basic fixes and suggestions are welcome here but I'd prefer to get this merged soon. To that end, there is an umbrella issue #367 and any additional features, tests, etc. should get their own issue added to that umbrella issue. Hopefully those smaller issues can then be doled out to individuals to work on.

See the notebook first for an example of what is going on.

Would appreciate basic comments and suggestions for features/testing from @joshwalawender @AnthonyHorton @lspitler @jamessynge. Mostly just on the overall idea. Please go ahead and create any related issues.

…/POCS into horizonFeatures

…a images) into the pocs examples notebooks folder.

…nto brendan-o-horizonFeatures

…nFeatures

* Build horizon line * Added example notebook

the new `Horizon` class * Updating some log output to be cleaner * Updating notebook * Removing sample from config file

wtgee · 2018-01-16T12:55:41Z

pocs/scheduler/constraint.py

@@ -146,11 +107,11 @@ def get_score(self, time, observer, observation, **kwargs):
        except KeyError:
            self.logger.error("Moon must be set")

-        moon_sep = moon.separation(observation.field.coord).value
+        moon_sep = observation.field.coord.separation(moon).value


I'm not sure why this changed, I'll check it out.

wtgee · 2018-01-16T13:03:52Z

pocs/observatory.py

                # Simple constraint for now
-                constraints = [MoonAvoidance(), Duration(30 * u.deg)]
+                constraints = [
+                    Altitude(horizon=horizon),


For now this will just create a flat line at the base horizon level so shouldn't affect current scheduler.

jamessynge

Looks good to me, but I'm not sure I fully understand how the scoring is performed.

jamessynge · 2018-01-17T02:12:59Z

pocs/scheduler/constraint.py


        # This would potentially be within image
        if moon_sep < 15:
-            self.logger.debug("Moon separation: {}".format(moon_sep))
+            self.logger.debug("\t\tMoon separation: {:.02f}".format(moon_sep))


\t\t seems unnecessary.

It helps with the output in this case and makes it consistent with the other scheduler items. If we don't want here we should remove everywhere in scheduler.

Remove everywhere, IMHO. ;-)

I've been guilty of using this approach in the past, and they stay long after they are useful. Perhaps it would be better to use a custom log colorizer.

Agreed about the tabs but I do think they are useful here. Long ago I had them all over the logs and it became a nightmare. Then I started thinking about how to write some logger wrapper that would auto-figure out the indent. Luckily I wisely abandoned the whole idea and removed all indents...except for here in the scheduler call.

I know @AnthonyHorton has used some in the SBIG and FLI code and I've been meaning to mention removing them. ;)

I'm going to leave in here unless very serious concerns (see notebook for how output looks) and again the additions here were to make the whole scheduler output consistent as it otherwise looked pretty bad.

Agree that we should try to avoid them elsewhere (i.e I get to be the exception but no one else. 😁).

I think you see the slippery slope all to well!

jamessynge · 2018-01-17T02:14:33Z

pocs/scheduler/constraint.py

@@ -159,3 +120,40 @@ def get_score(self, time, observer, observation, **kwargs):

    def __str__(self):
        return "Moon Avoidance"
+
+
+class Altitude(BaseConstraint):


The move down the file seems unnecessary, FWIW.

This is a direct pep8 change: https://www.python.org/dev/peps/pep-0008/#blank-lines

No, I mean the order of the classes was changed, with Altitude moved down the file. It was above class Duration, now is below.

Oh, got it. Yes, this was originally added to the bottom of the file as a new constraint named Horizon, but then I realized after writing it that it basically replaced the existing Altitude constraint (which wasn't being used at all) so I deleted the one from above and renamed the new one I had written.

Alphabetical order probably makes the most sense.

jamessynge · 2018-01-17T02:20:03Z

pocs/scheduler/constraint.py

+
+    def __init__(self, horizon=None, *args, **kwargs):
+        """
+        Retrieves the coordinate list from the config file and validates it


Doesn't appear to retrieve anything.

Oops, forgot to update docstring, thanks.

jamessynge · 2018-01-17T02:20:48Z

pocs/scheduler/dispatch.py

            for obs_name, observation in self.observations.items():
                if obs_name in valid_obs:
                    self.logger.debug("\tObservation: {}".format(obs_name))

                    veto, score = constraint.get_score(
                        time, self.observer, observation, **common_properties)

-                    self.logger.debug("\t\tScore: {}\tVeto: {}".format(score, veto))
+                    self.logger.debug("\t\tScore: {:.05f}\tVeto: {}".format(score, veto))


Again with the leading tabs. I suppose it makes it easier to find... sometimes...

jamessynge · 2018-01-17T02:21:26Z

pocs/observatory.py

@@ -734,8 +736,20 @@ def _create_scheduler(self):
                module = load_module(
                    'pocs.scheduler.{}'.format(scheduler_type))

+                obstruction_list = self.config['location'].get('obstructions', list())


Is there a description of the values in the list somewhere?

Just it the Horizon class. I haven't taken the step to add these to the config yet so will do that soon.

Maybe a comment here pointing to the Horizon class? Or a call to the Horizon class here, so that it is obvious where to look. E.g.

horizon = horizon_utils.load_horizon_from_config( self.config['location'], default_horizon=30.)

jamessynge · 2018-01-17T02:27:07Z

pocs/utils/images/horizon.py

@@ -0,0 +1,88 @@
+import numpy as np


I'm thinking about how to produce these from other info, like the azimuth and height above "ground" of the obstruction, and the height above ground of the mount/cameras (probably the lowest height).

You mean how to produce a Horizon automatically? That's partly what #364 is about.

Not precisely; I meant "get horizon from measurements" as opposed to "get horizon from image". If the latter is going to be easy to achieve, great. But if it will be a while, then I could ask one of the student's I'm working with to help with measuring the space so we can build the former. I could ask him about implementing it.

Regarding why this might be useful: the big obstruction next to PAN006 has a constant height, but its altitude at different azimuths will be very different. We could encode it with 5 measurements:

Height of wall.

Compass angle and distance to start of wall.

Compass angle and distance to end of wall.

Got it. We should capture that in a wiki page or document so have an Issue in #367.

this is a holdover from the branch from before astroplan switched how the moon works

…an-o-horizonFeatures

lspitler · 2018-01-18T00:29:06Z

Only aspect I don't see (it might be elsewhere in dispatch) is whether the field of view of the particular imaging system being used is factored into the horizon. For Huntsman this is tiny compared to Panoptes so we can go much closer to the horizon. Won't add a to-do for this as I don't know if it is already taken care of.

wtgee · 2018-01-18T00:30:51Z

Only aspect I don't see (it might be elsewhere in dispatch) is whether the field of view of the particular imaging system being used is factored into the horizon. For Huntsman this is tiny compared to Panoptes so we can go much closer to the horizon. Won't add a to-do for this as I don't know if it is already taken care of.

No, this is a good point and I mean to put an Issue in for it. We consider this to be the center of the image, which of course is not ideal for PANOPTES. I'll make the issue.

…an-o-horizonFeatures

codecov · 2018-01-19T08:58:28Z

Codecov Report

Merging #368 into develop will increase coverage by 0.39%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #368      +/-   ##
===========================================
+ Coverage    68.98%   69.38%   +0.39%     
===========================================
  Files           58       59       +1     
  Lines         4640     4690      +50     
  Branches       641      650       +9     
===========================================
+ Hits          3201     3254      +53     
+ Misses        1266     1265       -1     
+ Partials       173      171       -2

Impacted Files	Coverage Δ
pocs/scheduler/observation.py	`93.44% <ø> (ø)`	⬆️
pocs/utils/horizon.py	`100% <100%> (ø)`
pocs/scheduler/constraint.py	`96.05% <100%> (+1.6%)`	⬆️
pocs/observatory.py	`83.81% <100%> (+0.53%)`	⬆️
pocs/scheduler/dispatch.py	`98.03% <100%> (+1.96%)`	⬆️
pocs/utils/error.py	`94.23% <0%> (+0.23%)`	⬆️

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 055cf9c...95c73de. Read the comment docs.

…cal order)

wtgee · 2018-01-19T09:34:47Z

Going to merge this just because we definitely have a need on PAN001, PAN006, and Huntsman. :)

brendan-o and others added 24 commits November 25, 2016 13:38

Commented out modules

a7da97a

Merge remote-tracking branch 'upstream/develop' into develop

243d0ac

Added a new horizon test file

b655fbb

Working on testing the horizon class.

760752b

Fixed some pep8 style issues. Added the process_image method.

863b186

Improved the obstruction points validation tests

007a4c9

Fixed the testing.

2595491

Merge branch 'develop' into horizonFeatures

e17d7ec

Fixed py.test, docstrings.

b4f0f2f

Successfully implemented the config files

81aecff

Merge branch 'horizonFeatures' of https://github.com/brendanorenstein…

0ee03f3

…/POCS into horizonFeatures

Put the automating horizon limits notebook (and corresponding panarom…

8b2e0f7

…a images) into the pocs examples notebooks folder.

Merge branch 'develop' into horizonFeatures

6cfc017

Merge branch 'develop' into horizonFeatures

6bf9c4d

Merge branch 'horizonFeatures' of https://github.com/brendan-o/POCS i…

bc63846

…nto brendan-o-horizonFeatures

Adding changes from @brendan-o

cfa2bde

pep8 changes

94d0cec

Merge remote-tracking branch 'upstream/develop' into brendan-o-horizo…

9173ceb

…nFeatures

Updating horizon, WIP

9708d06

First cut

1a86f52

* Allow for numpy integers

1fa1e4a

* Build horizon line * Added example notebook

Removing unused (for now) code

c8c8915

Notebook updates

9703fc4

* Replacing existing (unused) Altitude constraint with one that uses

c705276

the new `Horizon` class * Updating some log output to be cleaner * Updating notebook * Removing sample from config file

wtgee added the scheduler label Jan 16, 2018

wtgee requested review from a team January 16, 2018 12:48

Small change so there is no change

8da8621

wtgee commented Jan 16, 2018

View reviewed changes

wtgee mentioned this pull request Jan 16, 2018

Horizon Constraints #367

Open

8 tasks

wtgee added 2 commits January 16, 2018 23:58

Removing old tests

df7fd1a

Fixing docstring

505d4bf

wtgee commented Jan 16, 2018

View reviewed changes

wtgee added 2 commits January 17, 2018 00:04

Changing Duration constraint to use default horizon

b491e30

Small docstring fix

c86ea20

wtgee mentioned this pull request Jan 16, 2018

Horizon features #85

Closed

jamessynge approved these changes Jan 17, 2018

View reviewed changes

wtgee added 3 commits January 17, 2018 13:42

Fix docstring

69ae1c3

Revert moon separation since I don't know why it was changed. I think

2e6d38b

this is a holdover from the branch from before astroplan switched how the moon works

Merge branch 'develop' of https://github.com/panoptes/POCS into brend…

1f85dc4

…an-o-horizonFeatures

wtgee added 2 commits January 18, 2018 12:45

Merge branch 'develop' of https://github.com/panoptes/POCS into brend…

37c199b

…an-o-horizonFeatures

Moving out of images subfolder and cleaning up some tests

4566d69

Moved Altitude constraint back to top (which puts them in alphabeti…

95c73de

…cal order)

wtgee merged commit d00eaae into panoptes:develop Jan 19, 2018

wtgee deleted the brendan-o-horizonFeatures branch January 19, 2018 09:39

wtgee mentioned this pull request Jan 24, 2018

Release 0.6.1 #415

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic horizon constraint features #368

Basic horizon constraint features #368

wtgee commented Jan 16, 2018 •

edited

Loading

wtgee Jan 16, 2018

wtgee Jan 16, 2018

jamessynge left a comment

jamessynge Jan 17, 2018

wtgee Jan 17, 2018 •

edited

Loading

jamessynge Jan 17, 2018 •

edited

Loading

wtgee Jan 18, 2018

jamessynge Jan 18, 2018

jamessynge Jan 17, 2018

wtgee Jan 17, 2018

jamessynge Jan 17, 2018

wtgee Jan 17, 2018

wtgee Jan 17, 2018

jamessynge Jan 17, 2018

wtgee Jan 17, 2018

jamessynge Jan 17, 2018

jamessynge Jan 17, 2018

wtgee Jan 17, 2018

jamessynge Jan 17, 2018 •

edited

Loading

jamessynge Jan 17, 2018

wtgee Jan 17, 2018

jamessynge Jan 17, 2018

jamessynge Jan 17, 2018

wtgee Jan 17, 2018

lspitler commented Jan 18, 2018

wtgee commented Jan 18, 2018

codecov bot commented Jan 19, 2018 •

edited

Loading

wtgee commented Jan 19, 2018

Basic horizon constraint features #368

Basic horizon constraint features #368

Conversation

wtgee commented Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamessynge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtgee Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

jamessynge Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamessynge Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lspitler commented Jan 18, 2018

wtgee commented Jan 18, 2018

codecov bot commented Jan 19, 2018 • edited Loading

Codecov Report

wtgee commented Jan 19, 2018

wtgee commented Jan 16, 2018 •

edited

Loading

wtgee Jan 17, 2018 •

edited

Loading

jamessynge Jan 17, 2018 •

edited

Loading

jamessynge Jan 17, 2018 •

edited

Loading

codecov bot commented Jan 19, 2018 •

edited

Loading