Make sequencer constraint #378

jermainegug · 2018-01-17T04:38:37Z

New PR for issue AstroHuntsman#5. This replaces #374.

Note: this is not to be merged yet! Much more work is to be done.

*make the Sequencer class *veto=True if current_obs is in obs_list *might want a better string to be returned

*return veto and score (does not need to be changed)

wtgee

Too easy. This is a much better approach than a different scheduler.

This looks good but we'll want to add some tests in a testing file that will ensure this is working. Take a look in pocs/tests/test_constraints.py and you can just add directly to that file.

To run just that test you can do "pytest -xv pocs/tests/test_constraints.py" at the command line.

wtgee · 2018-01-17T05:37:00Z

pocs/scheduler/constraint.py

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

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


Seqencer -> Sequencer

But let's maybe name that AlreadyVisited just to be really obvious. Sequencer seems to apply to more than one targets whereas a constraint technically only applies to one target at a time.

wtgee · 2018-01-17T05:39:11Z

pocs/scheduler/constraint.py

+    return veto, score * self.weight
+
+    def __str__(self):
+        return "Don't repeat observations"


How this gets used it's better to actually just have something like "AlreadyVisited". An example of how it shows up is with the word Altitude here:

I0116 12:30:21.285 dispatch.py:56 Checking Constraint: Altitude D0116 12:30:21.285 dispatch.py:59 Observation: Kepler 1100

*change name of method *change string returned by method

*small change to constraint *template of test **makes observed list **checks two different obs, one in list one not

wtgee · 2018-01-17T23:24:44Z

pocs/scheduler/constraint.py

@@ -170,10 +170,10 @@ def get_score(self, time, observer, observation, **kwargs):
        score = self._score

        target = observation.field
-        observed = observation.observed_list
+        observed_list = kwargs['observed_list']


For normal operations you'll still want to pull from observation.observed_list as that's how the scheduler builds it up. So do akwargs.get('observed_list', observation.observed_list) here.

wtgee · 2018-01-17T23:26:52Z

pocs/tests/test_constraints.py

+    observed_list[observation1.seq_time] = observation1
+    observed_list[observation2.seq_time] = observation2
+
+    veto1, score1 = avc.get_score(time, observer, observation1, observed_list=observed_list)


An alternative to passing the observed_list and using the kwargs as above is to just alter the observer.observed_list here.

wtgee · 2018-01-17T23:28:42Z

The MoonAvoidance and Duration are bad examples, but can you add a docstring similar to Altitude that describes what this does: https://github.com/jermainegug/POCS/blob/2260d04f5fdaef8135082da61c814ae99678f722/pocs/scheduler/constraint.py#L38

*add docstring *use observer.observed_list *change test to use observer.observed_list

jermainegug · 2018-01-17T23:45:15Z

Haven't been able to test it yet... Windows problems.

wtgee · 2018-01-17T23:51:04Z

You can check the travis build here on the PR. You have some spacing/indent issues it is saying.

jermainegug · 2018-01-17T23:54:16Z

OK, cool. I will just use that to check issues.

*consistent formatting *add import OrderedDict to test_constraints.py

wtgee · 2018-01-18T02:28:28Z

pocs/scheduler/constraint.py

-        if target in observed_list:
-            veto = True
+        for seq_time, Observation in observed_list:
+            if target = observation:


You are assigning (=) here instead of comparing (==). Which might not show an error.

wtgee · 2018-01-18T02:29:32Z

pocs/scheduler/constraint.py

        observed_list = kwargs.get('observed_list', observer.observed_list)

-        if target in observed_list:
-            veto = True
+        for seq_time, Observation in observed_list:


Observation -> previous_obs You already have an observation object above so can't just use observation (no capital letters).

*add .items() at the end of observed_list

jermainegug · 2018-01-18T04:36:30Z

OK, so I know what it going on now with the OrderedDict(). For some reason, which I can't find any explanation for, when I do this:

observer.observed_list[observation1.seq_time] = observation1
observer.observed_list[observation2.seq_time] = observation2

The previous entries are over written by a recent entry. I will try to just assign it as a normal dictionary but I can't see why he ordered dictionary is behaving in such a way.
If the same thing happens with a normal dictionary then I am completely lost.

wtgee · 2018-01-18T04:40:21Z

The previous entries are over written by a recent entry. I will try to just assign it as a normal dictionary but I can't see why he ordered dictionary is behaving in such a way.

Not sure I understand what you mean. But the observed_list is an OrderedDict so that is what the constraint will be given, so that is what needs to be tested.

wtgee · 2018-01-18T04:43:46Z

Ah, I see. The Observation requires you to set a seq_time. You are not doing that so the seq_time is None for both entries so they get stored under the same dictionary key. You need something like

observation1.seq_time = some_time
observation1.seq_time = some_different_time

then put it in the OrderedDict

jermainegug · 2018-01-18T04:45:03Z

Ahh, of course!

*set seq_time for all observations

codecov · 2018-01-18T05:07:54Z

Codecov Report

Merging #378 into develop will decrease coverage by 0.03%.
The diff coverage is 92.3%.

@@             Coverage Diff             @@
##           develop     #378      +/-   ##
===========================================
- Coverage    69.08%   69.05%   -0.04%     
===========================================
  Files           60       60              
  Lines         4723     4757      +34     
  Branches       652      656       +4     
===========================================
+ Hits          3263     3285      +22     
- Misses        1289     1297       +8     
- Partials       171      175       +4

Impacted Files	Coverage Δ
pocs/scheduler/dispatch.py	`98.03% <ø> (ø)`	⬆️
pocs/scheduler/constraint.py	`95.5% <92.3%> (-0.55%)`	⬇️
pocs/utils/logger.py	`89.55% <0%> (-6.94%)`	⬇️
pocs/dome/astrohaven.py	`71.3% <0%> (-2.57%)`	⬇️
pocs/utils/error.py	`92.3% <0%> (-1.93%)`	⬇️
pocs/observatory.py	`83.52% <0%> (-0.29%)`	⬇️
pocs/utils/rs232.py	`92.03% <0%> (ø)`	⬆️
pocs/images.py	`92.52% <0%> (+0.07%)`	⬆️
pocs/utils/images/__init__.py	`83.6% <0%> (+0.84%)`	⬆️

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 d00eaae...8072579. Read the comment docs.

jermainegug · 2018-01-18T05:11:25Z

Now we just need to implement this and we are good to merge!

wtgee · 2018-01-18T05:35:38Z

pocs/scheduler/constraint.py

+
+        observed_list = kwargs.get('observed_list', observer.observed_list)
+
+        for seq_time, previous_obs in observed_list.items():


Actually, now that the OrderedDict is sorted out I think you can switch this back to if observation.seq_time in observed_list and avoid the loop.

Awesome, didn't like the idea of the loop but I remembered that seq_time will be different for each `observation.

wtgee

This is looking good. One last request: can you now update the Scheduler Playground notebook and add this constraint in cell 4? Then just run the rest of the notebook the same. Down in the Entire Night Simulation it currently selects Wasp-35 twice (once before the meridian and once after) and this update should change that.

Note that the order you add the constraints matters and you want this one first. That is, you want this constraint to veto something initially so that it will not even be considered by the other constraints.

jermainegug · 2018-01-18T10:37:13Z

Thank you! No problem, will do that soon. Good that we have Wasp-35 twice so we can make sure that it works properly in the Scheduler Playground.

Of course, makes sense to make sure we aren't checking conditions that we won't be using.

*include `AlreadyVisited` constraint *import from constraint *add to `constrains` array

wtgee · 2018-01-18T22:15:08Z

@jermainegug Can you run all the cells in the notebook and then save and commit it? I want to be able to see the output in the bottom of the notebook where it vetoes targets because of constraint.

wtgee · 2018-01-19T09:00:41Z

pocs/scheduler/constraint.py

+        veto = False
+        score = self._score
+
+        observed_list = kwargs.get('observed_list', observer.observed_list)


I told you wrong here. observed_list belongs to the scheduler, not to the observer (the observer also belongs to the scheduler). You can remove the observer.observed_list here and just fetch from kwargs. Then in the dispatch.py scheduler you can pass the observed_list in as a common_properties (here):

common_properties = { 'end_of_night': self.observer.tonight(time=time, horizon=-18 * u.degree)[-1], 'moon': get_moon(time, self.observer.location), 'observed_list': self.observed_list }

The common_properties get passed into as kwargs down below that.

But this means we should add a test. You have added tests to successfully cover just the constraint, now we should add something in test_dispatch_scheduler.py that uses it as part of a normal scheduling routine.

*take observer.observed_list out constraint *add observed_list to common_properties *just define observed_list in test (?)

jermainegug · 2018-01-19T11:36:33Z

How can I make the test_already_visited method work?

wtgee · 2018-01-19T21:28:01Z

pocs/tests/test_constraints.py

+    observed_list[observation1.seq_time] = observation1
+    observed_list[observation2.seq_time] = observation2
+
+    veto1, score1 = avc.get_score(time, observer, observation1)


In dispatch.py you can see how common properties is passed to get_score by using the **common_properties syntax, which passes individual keys of the dictionary to the method (as opposed to passing the dictionary itself).

So in your get_score here you just need to pass the observed_list with a keyword, so observed_list=observed_list. Then the generic kwargs in the get_score you wrote will have it available as kwargs.get('observed_list'). This is one of the points of using **kwargs as a parameter, it can pick up keyword arguments that are not otherwise explicitly stated. We have a get_score in all of the constraints and not all the keywords apply to all the constraint types so this is how you design for something like that.

Ahh, interesting. I wasn't too sure how **kwargs worked but that makes plenty of sense. Thanks!

*take out observed_list=observed_list from get_score *was put into the wrong get_score

wtgee · 2018-01-21T01:46:10Z

pocs/scheduler/constraint.py

+
+        observed_list = kwargs.get('observed_list')
+
+        if observation.seq_time in observed_list:


I had told you wrong (again). By definition the seq_time is going to be unique for each observation even if they are for the same field. What we want to check is the field, via

observed_field_list = [obs.field for obs in observed_list.values()] if observation.field in observed_field_list:

I tested that out in the notebook copied from all your latest code and it finally outputs something like I would expect. It still dwells on one field but does not reselect a field after it has deselected it.

jermainegug added 3 commits January 17, 2018 15:35

Make sequencer constraint

1ebfa58

*make the Sequencer class *veto=True if current_obs is in obs_list *might want a better string to be returned

*can't use scheduler here (try observation)

497b4dd

*return veto and score (does not need to be changed)

*edit

7623a69

wtgee reviewed Jan 17, 2018

View reviewed changes

jermainegug added 2 commits January 17, 2018 23:43

Change names

beac373

*change name of method *change string returned by method

Start making test for constraint

2260d04

*small change to constraint *template of test **makes observed list **checks two different obs, one in list one not

wtgee reviewed Jan 17, 2018

View reviewed changes

Edits

b9f7877

*add docstring *use observer.observed_list *change test to use observer.observed_list

jermainegug added 7 commits January 18, 2018 10:55

*fix indent typo

440a466

*put return into function

f4ab18e

*import OrderedDict

cfce422

*initialize observed_list to be empty

c8e118e

Couple fixups

0c72b44

*consistent formatting *add import OrderedDict to test_constraints.py

*not checking observation.field

86c6f55

*try for loop

71735ab

wtgee reviewed Jan 18, 2018

View reviewed changes

jermainegug added 3 commits January 18, 2018 13:37

*fix typos

4548b9b

*put print to see what observed_list looks like

8106c11

Hopefully final fix!

dcfba36

*add .items() at the end of observed_list

*try dict instead of OrderedDict

aa6abe0

jermainegug added 2 commits January 18, 2018 15:48

*change back to OrderedDict

b50a07d

*set seq_time for all observations

*add sentence in class description

e33509e

*don't need import OrderedDict in constraint.py

773d2b3

wtgee reviewed Jan 18, 2018

View reviewed changes

*avoid loop

25b1a3e

wtgee reviewed Jan 18, 2018

View reviewed changes

Edit the scheduler playground

5b7b6fe

*include `AlreadyVisited` constraint *import from constraint *add to `constrains` array

wtgee reviewed Jan 19, 2018

View reviewed changes

jermainegug and others added 2 commits January 19, 2018 21:43

Fixing issue with observed_list

5c0b274

*take observer.observed_list out constraint *add observed_list to common_properties *just define observed_list in test (?)

Merge branch 'develop' into make-new-constraint

732e041

wtgee reviewed Jan 19, 2018

View reviewed changes

jermainegug and others added 4 commits January 20, 2018 11:06

*pass observed_list into get_score for already visited constraint

fd78307

*put observed_list into get_score

058bc65

Undo commit

f2cba6c

*take out observed_list=observed_list from get_score *was put into the wrong get_score

*update shceduler notebook

37c9616

wtgee requested changes Jan 21, 2018

View reviewed changes

*use field instead of seq_time

8072579

wtgee approved these changes Jan 21, 2018

View reviewed changes

wtgee merged commit 9d6fe5b into panoptes:develop Jan 21, 2018

jermainegug deleted the make-new-constraint branch January 21, 2018 02:34

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

Make sequencer constraint #378

Make sequencer constraint #378

jermainegug commented Jan 17, 2018 •

edited

Loading

wtgee left a comment •

edited

Loading

wtgee Jan 17, 2018

wtgee Jan 17, 2018

wtgee Jan 17, 2018

wtgee Jan 17, 2018

wtgee commented Jan 17, 2018

jermainegug commented Jan 17, 2018

wtgee commented Jan 17, 2018

jermainegug commented Jan 17, 2018

wtgee Jan 18, 2018

wtgee Jan 18, 2018

jermainegug commented Jan 18, 2018

wtgee commented Jan 18, 2018

wtgee commented Jan 18, 2018

jermainegug commented Jan 18, 2018

codecov bot commented Jan 18, 2018 •

edited

Loading

jermainegug commented Jan 18, 2018

wtgee Jan 18, 2018

jermainegug Jan 18, 2018

wtgee left a comment

jermainegug commented Jan 18, 2018

wtgee commented Jan 18, 2018

wtgee Jan 19, 2018

wtgee Jan 19, 2018

jermainegug commented Jan 19, 2018

wtgee Jan 19, 2018 •

edited

Loading

jermainegug Jan 19, 2018

wtgee Jan 21, 2018 •

edited

Loading


		observed_list = kwargs.get('observed_list', observer.observed_list)

		for seq_time, previous_obs in observed_list.items():


		observed_list = kwargs.get('observed_list')

		if observation.seq_time in observed_list:

Make sequencer constraint #378

Make sequencer constraint #378

Conversation

jermainegug commented Jan 17, 2018 • edited Loading

wtgee left a comment • 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

wtgee commented Jan 17, 2018

jermainegug commented Jan 17, 2018

wtgee commented Jan 17, 2018

jermainegug commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jermainegug commented Jan 18, 2018

wtgee commented Jan 18, 2018

wtgee commented Jan 18, 2018

jermainegug commented Jan 18, 2018

codecov bot commented Jan 18, 2018 • edited Loading

Codecov Report

jermainegug commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtgee left a comment

Choose a reason for hiding this comment

jermainegug commented Jan 18, 2018

wtgee commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jermainegug commented Jan 19, 2018

wtgee Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtgee Jan 21, 2018 • edited Loading

Choose a reason for hiding this comment

jermainegug commented Jan 17, 2018 •

edited

Loading

wtgee left a comment •

edited

Loading

codecov bot commented Jan 18, 2018 •

edited

Loading

wtgee Jan 19, 2018 •

edited

Loading

wtgee Jan 21, 2018 •

edited

Loading