Use numpy broadcasting for altaz and constraints by StuartLittlefair · Pull Request #286 · astropy/astroplan

StuartLittlefair · 2017-02-04T17:41:44Z

As discussed in #285 I think there is an advantage to relying on the broadcasting of times and targets in astropy, now that those objects are shape aware. This PR is basically to show how that would work, so the code maintainers can discuss if it's a good idea or not.

This PR passes all tests locally (some of which have been tweaked to accommodate changed behaviour - see below). To do this I have included changes in #281, #279, #273 as well.

The three potential gains are cleaner code, faster performance and greater flexibility. The downsides are that it requires astropy v1.3, and that it's a change which will probably break user code - see discussion below.

Cleaner Code

The philosophy is to always convert target arguments to SkyCoord objects. As a result, we no longer have to have two paths through the code for vector and non-vector targets. We can also get rid of constructions like:

target_coos = [target.coord if hasattr(target, 'coord') else target
                         for target in targets]
target_altazs = [observer.altaz(times, coo) for coo in target_coos]
solar_separation = Angle([sunaltaz.separation(taa) for taa in target_altazs])

which becomes

solar_separation = targets.separation(sunaltaz)

Faster performance

Performance is similar for single targets and single or a few times. However, when calculating the altaz or evaluating constraints over many targets and times it is much faster. For example evaluating the built-in constraints for 100 times and 2 targets has improved from 15s to 1.5s. Scheduling currently does every combination of target/time one-by-one so doesn't see any speed-up but the code could be adapted in a future PR.

Greater flexibility

This is also where the code-breaking behaviour arises. Previously, evaluating a constraint or calculating altaz for N targets and M times would always return an NxM array. This is no longer true. If N and M are equal, for example, the result will be a 1D array of size M.

I'd argue this is an improvement - one doesn't always want to calculate every target at every time. To minimise the transition pain, both altaz and the constraint call function have a grid argument, which restores the previous behaviour.

StuartLittlefair · 2017-02-04T19:29:12Z

OK, I was going to ping people when this was passing tests, but I can't understand some of the failures - in particular I'm not sure what to do about the sphinx failures.

The older version of numpy also seem to fail because Time has no len attribute. Not sure what to do about that.

So, for discussion, @eteq, @bmorris3, @bsipocz you might want to take a look at this PR sometime.

StuartLittlefair · 2017-02-24T16:03:43Z

I have now rebased this so it is easier to see what has changed. I've also profiled the changes here against the current master which also uses SkyCoords internally. I evaluated each constraint for 100 times and 3 targets, and also for each combination of scalar time, vector target and vice versa.

This PR is faster, but not dramatically so (with the exception of the MoonSeparationConstraint.

The results (in s) are shown below:

Constraint	Master	This PR
Altitude	0.35	0.27
Airmass	0.06	0.09
AtNight	0.19	0.21
SunSeparation	1.07	0.53
MoonSeparation	29.46	1.09
MoonIllumination	0.76	0.59
LocalTime	0.02	0.09
Time	0.01	0.11

bmorris3 · 2017-04-26T22:16:06Z

@StuartLittlefair It looks like this PR is good to go! I just want to check before I click the green button that this one is ready to merge?

StuartLittlefair · 2017-04-27T06:19:17Z

I'm happy with it.

I'd be happier if @eteq took a look at it because it's a big change. Also, does it need a change log entry?

bmorris3 · 2017-04-27T06:22:05Z

Ok, then I'm going to do an intermediate release first, so that we can get v0.2.1 on pypi ASAP.

StuartLittlefair · 2017-04-27T06:23:54Z

Good plan!

StuartLittlefair · 2017-05-09T16:15:51Z

Hi @bmorris3.

I'm thinking of finishing up this PR, but I can't make my mind up about the default behaviour. In the past astroplan has always done calculations for a grid of times and targets and as you know, this PR changes this default. It doesn't have to; we could make gridding the default again, and allow non gridding as an option.

Edit: my own opinion is that the intuitive behaviour is the one in this PR; if I supply three times and three targets to altaz (for example), I kind of expect three altaz results...

I'm going to finish up editing the docs and adding a changelog over the next week, but before I do that I wanted to check that you are happy with this decision?

EDIT: this PR is now ready to go as far as I'm concerned. It just really needs package maintainers to agree on the API breaking change.

…argets' in constraints

StuartLittlefair · 2017-05-12T00:09:50Z

p.s, this PR fixes #296

StuartLittlefair · 2017-05-25T14:25:16Z

@bmorris3, @eteq - can I nudge you for a code review of this PR? I'm already starting to use it in our own telescope control/scheduling package due to the speedups involved in not gridding targets and times where not needed.

bmorris3 · 2017-06-07T21:21:48Z

@StuartLittlefair sorry for the long delay there! I just finished my candidacy exam and can take a look now. Reviewing!

bmorris3 · 2017-06-07T21:32:33Z

-To see the (target x time) array for the non-boolean score::
+By default, calling a constraint will try to broadcast the time and target arrays
+against each other, and raise a `ValueError` if this is not possible. To see the
+(target x time) array for the constraint, there is an optional "grid_times_targets"


Small correction – would you please make "grid_times_targets" -> `grid_times_targets`? That way it will be rendered nicely in the docs.

bmorris3 · 2017-06-07T21:33:13Z

@@ -3,6 +3,13 @@

 - No changes yet


We can remove this line with your addition below.

StuartLittlefair · 2017-06-08T11:46:20Z

@bmorris3 congratulations!

I've made the minor changes requested

StuartLittlefair · 2017-06-08T14:53:08Z

Closed and re-opened to start tests, which failed due to random lookup failure for APO site.

bmorris3 · 2017-06-08T15:36:10Z

Thanks for all of your hard work @StuartLittlefair!

StuartLittlefair closed this Feb 6, 2017

StuartLittlefair reopened this Feb 6, 2017

This was referenced Feb 9, 2017

moon_phase_angle slow for large time arrays #211

Closed

Fix broadcasting by using vector SkyCoords internally [WIP] #285

Merged

StuartLittlefair force-pushed the use_broadcasting branch from a52920d to 48ee0d0 Compare February 23, 2017 16:29

StuartLittlefair added 18 commits May 11, 2017 20:37

first pass at full broadcasting

2251ca9

remove automatic gridding of targets and times

1fb0d59

restore pure broadcasting of times and targets

820764e

changed tests to reflect new behaviour

b1b2326

small simplification of horiz_cross code

a7e095c

minor fix to handle no targets

6ebbe7a

use numpy broadcasting in constraints

ff3d93c

update schedules to work w/ broadcast constraints

db41ff3

minor changes to tests

0635800

doc changes for constraints

6b1aae6

speed up broadcasting

ab96792

convert scorer arguments to skycoord on creation

b755bbd

handle None-type targets

f2093d8

remove numpy broadcast to

c83a768

changed docstring formatting

5d271b3

isolate warning catching to one line

5906840

compatibility with scalar times

87beff1

scheduler tweak after AirmassConstraint change

ea1fe5f

StuartLittlefair added 8 commits May 11, 2017 20:48

fixing hashing again

1667525

faster key for caches

8816dd1

rename 'grid' argument in observer for consistency with 'grid_times_t…

ae26caa

…argets' in constraints

removed vscode database

f0f6875

minor bugfix

81e2641

caches should respect shape

e9bf6f7

improved documentation

703f87b

added changelog entry

2c539f3

StuartLittlefair force-pushed the use_broadcasting branch from 4a47f60 to 2c539f3 Compare May 11, 2017 23:52

simpler test syntax

d1f2c2f

StuartLittlefair mentioned this pull request May 12, 2017

Vectorised constraints #190

Closed

StuartLittlefair added 2 commits May 12, 2017 11:43

altaz should respect grid_times_targets arg always

40916c7

everything relevant gets a grid_times_targets arg

8c47a4d

StuartLittlefair force-pushed the use_broadcasting branch from 273d0a6 to 8c47a4d Compare May 12, 2017 18:28

martinjohndyer mentioned this pull request May 19, 2017

The survey scheduler GOTO-OBS/gtecs-control#54

Closed

bmorris3 approved these changes Jun 7, 2017

View reviewed changes

bmorris3 reviewed Jun 7, 2017

View reviewed changes

doc changes for bmorris

dcbce13

fixed typo

0189baf

StuartLittlefair closed this Jun 8, 2017

StuartLittlefair reopened this Jun 8, 2017

bmorris3 merged commit 42e3607 into astropy:master Jun 8, 2017

StuartLittlefair deleted the use_broadcasting branch August 18, 2023 10:50

Conversation

StuartLittlefair commented Feb 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Cleaner Code

Faster performance

Greater flexibility

Uh oh!

StuartLittlefair commented Feb 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StuartLittlefair commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmorris3 commented Apr 26, 2017

Uh oh!

StuartLittlefair commented Apr 27, 2017

Uh oh!

bmorris3 commented Apr 27, 2017

Uh oh!

StuartLittlefair commented Apr 27, 2017

Uh oh!

StuartLittlefair commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StuartLittlefair commented May 12, 2017

Uh oh!

StuartLittlefair commented May 25, 2017

Uh oh!

bmorris3 commented Jun 7, 2017

Uh oh!

bmorris3 Jun 7, 2017

Choose a reason for hiding this comment

Uh oh!

bmorris3 Jun 7, 2017

Choose a reason for hiding this comment

Uh oh!

StuartLittlefair commented Jun 8, 2017

Uh oh!

StuartLittlefair commented Jun 8, 2017

Uh oh!

bmorris3 commented Jun 8, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

StuartLittlefair commented Feb 4, 2017 •

edited

Loading

StuartLittlefair commented Feb 4, 2017 •

edited

Loading

StuartLittlefair commented Feb 24, 2017 •

edited

Loading

StuartLittlefair commented May 9, 2017 •

edited

Loading