Fix broadcasting by using vector SkyCoords internally [WIP] by StuartLittlefair · Pull Request #285 · astropy/astroplan

StuartLittlefair · 2017-02-02T09:55:19Z

As described in #215, I think there's an argument for all internal calculations converting their arguments to Time and SkyCoord objects, which may be vector. This PR builds on the utility function in #215 and implements this functionality. It's a large change in the observer codebase, but outward facing functionality should be compatible with how it was before. However, there are so many unassociated test failures from other issues with astropy 1.3 it's hard to know, hence the WIP designation.

Once complete, this PR could fix remaining broadcasting issues with astropy 1.3 (e.g #282, #263, #272).

Some notes:

This PR requires astropy 1.3 as a minimum version.
~~Although behaviour for scalar times and targets is unchanged, there is an improvement (IMO) in functionality for multiple times and targets. I briefly describe that here.~~

This PR should give identical behaviour to the existing astroplan code, though the internals of the observer module have changed quite a lot. Please disregard the code samples below - I've left them in the comment for historical purposes

First - for scalar objects:

In [2]:  apo = Observer.at_site("APO")
In [3]:  times = Time(2457786.9048868627, format='jd')
In [4]:  m31 = coord.SkyCoord.from_name('M31')
In [5]:  apo.altaz(times, m31)
Out[5]: 
<SkyCoord (AltAz: obstime=2017-02-02 09:43:02.224949, location=(-1463969.3018517173, -5166673.342234327, 3434985.7120456537) m, pressure=0.0 hPa, temperature=0.0 deg_C, relative_humidity=0, obswl=1.0 micron): (az, alt) in deg
    ( 346.02166236, -13.98494769)>

However, if the times and targets are 1D and broadcastable, altaz and similar calcs will broadcast automatically:

In [6]: ippeg = coord.SkyCoord.from_name('IP Peg')
In [7]: deneb = coord.SkyCoord.from_name('Deneb')
In [8]: times = times + np.linspace(-0.3, 0.3, 3)*u.d
In [9]: apo.altaz(times, [m31, ippeg, deneb])
Out[9]: 
<SkyCoord (AltAz: obstime=[datetime.datetime(2017, 2, 2, 2, 31, 2, 224949)
 datetime.datetime(2017, 2, 2, 9, 43, 2, 224949)
 datetime.datetime(2017, 2, 2, 16, 55, 2, 224949)], location=(-1463969.3018517173, -5166673.342234327, 3434985.7120456537) m, pressure=0.0 hPa, temperature=0.0 deg_C, relative_humidity=0, obswl=1.0 micron): (az, alt) in deg
    [( 297.48755926,  47.14479827), (   2.09385352, -38.68372101),
     (  52.36092567,  64.09699143)]>

also works with lists of FixedTargets

In [11]: apo.altaz(times, [FixedTarget(targ) for targ in [m31, ippeg, deneb]])
Out[11]: 
<SkyCoord (AltAz: obstime=[datetime.datetime(2017, 2, 2, 2, 31, 2, 224949)
 datetime.datetime(2017, 2, 2, 9, 43, 2, 224949)
 datetime.datetime(2017, 2, 2, 16, 55, 2, 224949)], location=(-1463969.3018517173, -5166673.342234327, 3434985.7120456537) m, pressure=0.0 hPa, temperature=0.0 deg_C, relative_humidity=0, obswl=1.0 micron): (az, alt) in deg
    [( 297.48755926,  47.14479827), (   2.09385352, -38.68372101),
     (  52.36092567,  64.09699143)]>

So far the same. But using vector skycoords internally adds functionality, since the horizon argument to the observer routines can be non-scalar and it will work as long as you can broadcast the horizon and altaz results:

In [12]: apo.target_is_up(times, [m31, ippeg, deneb], horizon=u.Quantity([50*u.deg, 0*u.deg, 20*u.deg]))
Out[12]: array([False, False,  True], dtype=bool)
In [13]: apo.target_is_up(times, [m31, ippeg, deneb])
Out[13]: array([ True, False,  True], dtype=bool)

Also, by manipulating the shape of the times and targets object you can create a grid of altaz at all times and targets, rather than broadcasting, e.g:

In [19]: apo.altaz(times[:, np.newaxis], [m31, ippeg, deneb])
Out[19]: 
<SkyCoord (AltAz: obstime=[[datetime.datetime(2017, 2, 2, 2, 31, 2, 224949)
  datetime.datetime(2017, 2, 2, 2, 31, 2, 224949)
  datetime.datetime(2017, 2, 2, 2, 31, 2, 224949)]
 [datetime.datetime(2017, 2, 2, 9, 43, 2, 224949)
  datetime.datetime(2017, 2, 2, 9, 43, 2, 224949)
  datetime.datetime(2017, 2, 2, 9, 43, 2, 224949)]
 [datetime.datetime(2017, 2, 2, 16, 55, 2, 224949)
  datetime.datetime(2017, 2, 2, 16, 55, 2, 224949)
  datetime.datetime(2017, 2, 2, 16, 55, 2, 224949)]], location=(-1463969.3018517173, -5166673.342234327, 3434985.7120456537) m, pressure=0.0 hPa, temperature=0.0 deg_C, relative_humidity=0, obswl=1.0 micron): (az, alt) in deg
    [[( 297.48755926,  47.14479827), ( 277.5507293 ,  23.53321076),
      ( 319.5051457 ,   8.36398593)],
     [( 346.02166236, -13.98494769), (   2.09385352, -38.68372101),
      (  28.22799217,  -3.00973326)],
     [(  53.52265937,  21.03003458), (  84.15855299,  26.41118594),
      (  52.36092567,  64.09699143)]]>

Although I haven't timed the performance I suspect it will be faster than the old, non-vectorised code as well.

There are still some issues with using vector Times or SkyCoords with higher dimensionality - see TODO note in _horiz_cross.

StuartLittlefair · 2017-02-02T09:57:20Z

@bsipocz Can you take a look at this PR? I've so many other issues with my local build (the remote test issues etc) that I can't test it properly, but it may address the broadcasting issues. Is there an easy way to make a local copy of a branch with all the prior fixes so I can test thoroughly? I'm a bit git-challenged!

@bmorris3 - this is a really big change to the code, so you probably want to take a look as well.

bsipocz · 2017-02-02T10:19:18Z

@StuartLittlefair - Sure, I've merged this into the testing branch, test are running here: https://travis-ci.org/bsipocz/astroplan/builds/197593459
There were quite a few conflicts, but hopefully I got them resolved properly.

bsipocz · 2017-02-02T10:23:23Z

The docstring issues seems real, what do you think? https://travis-ci.org/bsipocz/astroplan/jobs/197593463#L759

bsipocz · 2017-02-02T10:31:45Z

Ohh, and I forgot to answer the question: usually for testing multiple PRs, I usually open a new separate test branch and merge in the PRs there. If you have travis turned on your fork, this would do the tests there.

StuartLittlefair · 2017-02-02T10:37:05Z

I think the failures are real.

There's something that needs discussion here - in previous versions of astroplan if you called altaz with M targets and N times (M /= N), you would get an (M, N) array back. Also, if you called altaz with a list of times and single target you'd get a (1, N) array.

My PR has changed that behaviour, which has caused some failures in the constraints and scheduling code. I could re-instate it so that altaz worked as before but I would argue the way it works in this PR is an improvement, and it's better the fix the scheduling and constraints code to work with the new behaviour.

This does cause the issue that some existing user code might break, but there is still a healthy API warning on the astroplan main page...

This is not the current default behaviour

bsipocz · 2017-02-02T10:38:17Z

+
+    coo = get_skycoord(m31)
+    assert coo.is_equivalent_frame(ICRS())
+    assert_raises(TypeError, len, coo)


Could you please use pytest.raises instead? We don't have nose as a mandatory requirement but pytest, and this currently fails with ImportError: Need nose >= 1.0.0 for tests - see http://somethingaboutorange.com/mrl/projects/nose

bsipocz · 2017-02-02T10:40:30Z

OK, I see. For that discussion I think we need @bmorris3 and @eteq, as I'm not aware of the details for any decision behind the original API.

StuartLittlefair · 2017-02-02T11:10:38Z

@bsipocz given the currently broken state of astroplan with astropy 1.3, what do you think about making this PR keep the original behaviour, and submit another PR later for the API change?

bsipocz · 2017-02-02T11:39:29Z

Sounds like a good approach to me.

StuartLittlefair · 2017-02-02T14:59:11Z

OK - so the version pushed now restores the original behaviour of astroplan, at least as far as it comes to passing the tests. There may be some cases I've not reverted properly - e.g sun_rise_time called with N times returns an NxN grid.

A local test branch with #281, #279, #273 and this PR applied seem to pass tests, with the exception of one failure in scheduler which is (I think) unrelated.

bsipocz · 2017-02-02T15:02:41Z

Testing this with travis now here: https://travis-ci.org/bsipocz/astroplan/builds/197668370

bsipocz · 2017-02-02T15:13:36Z

There are still failures, one is the usage of nose (see inline comment), the other one is unrelated and I can fix in another PR.

StuartLittlefair · 2017-02-02T15:38:13Z

thanks @bsipocz - fixed the nose issue.

bsipocz · 2017-02-02T15:39:07Z

@StuartLittlefair - Thanks a lot, quite good to see that everything is back to normal.

pinging @olebole and @ViviCoder as they may want to use this to patch the debian version

bmorris3 · 2017-02-02T16:13:12Z

@StuartLittlefair Unfortunately there's a little rebase that needs to occur after an update to some Observer docstrings. Still seem to be some build failures, too.

StuartLittlefair · 2017-02-02T16:24:11Z

@bmorris3 I've just rebased. I don't think this has any chance of passing until #281, #279, #273 are also merged...

StuartLittlefair · 2017-02-09T20:01:28Z

@bmorris3 It might be nice to make astroplan work with the latest version of astropy. I've got two PRs which do that in principle, this and #286. Both only work with astropy v1.3, but #286 could break some user-facing code.

#286 is nearly passing tests - just some docstring issues I can't work out.

OTOH, this PR doesn't include the PRs mentioned in the thread above, so will never pass tests. Those PRs also can't be merged, since they don't pass tests! I'd appreciate some advice on the next step. Is it a good idea to merge in the changes in #281, #279, #273 and get this PR passing?

bsipocz · 2017-02-09T23:09:40Z

@StuartLittlefair @bmorris3 - I think we are already at a point when we need to merge all the bugfix PRs without any of the individual ones passing all the tests. There were already a few tests branches that included all those PRs, if you like I can rerun the tests on the latest one to see whether there are any new divergences compared to astropy dev.

StuartLittlefair · 2017-02-09T23:16:08Z

@bsipocz - I think that would be really helpful.

bsipocz · 2017-02-10T01:39:30Z

OK, so I've rebased the test branch, and merge this on in, tests are here:
https://travis-ci.org/bsipocz/astroplan/builds/200202721

We have the a TargetAlwaysUpWarning, and a real failure with numpy dev, and a few sphinx warnings left.

StuartLittlefair · 2017-02-10T21:29:23Z

@bsipocz so I've worked out how to make a test branch on my own repository :)

With the latest commits I think I've got all the failures except some of the sphinx warnings. In particular I'm not sure what to do about the OldEarthOrientationWarning . I'm also not sure if the latex-related warnings in the sphinx build are important, but they are part of astropy helpers.

Can you merge my latest commits to my test branch and see if you agree with the summary above. Also, any idea what to do about the OldEarthOrientationWarnings?

bsipocz · 2017-02-10T22:00:55Z

@StuartLittlefair - Latest commits from here or from #286? We had the OldEarth... before, but only had some workarounds for it never a proper solution.
There is an astropy-helpers update PR, too, the latex warnings should go away with that.

StuartLittlefair · 2017-02-10T22:03:17Z

@bsipocz from here.

I'm tracking fixes in this PR over there. My hope is that this (or a branch with these commits) can be merged, and I can rebase #286 so the changes are clearer.

bsipocz · 2017-02-11T20:33:50Z

Done. Tests are here: https://travis-ci.org/bsipocz/astroplan/builds/200726025

StuartLittlefair · 2017-02-11T21:26:44Z

@bsipocz do my eyes deceive me or is this a passing branch?

What's the next step? Presumably one PR based on all the commits that make up your test branch?

bsipocz · 2017-02-11T21:32:19Z

Wow, I haven't noticed, copied the link here while it was still running.
Actually I would prefer the individual PRs being merged as a big joint PR will mess up any future bug hunts, etc.
Next step is to get the attention of someone with commit rights, and if they want a big joint PR, I can open it straight away from that passing branch ;)

StuartLittlefair · 2017-02-11T21:36:35Z

@bmorris3 @eteq

Thanks to the hard work of @bsipocz we now have a collection of PRs (this one included) which in combination give a passing test suite!

Note that contrary to what is said in the initial message with this PR, it does not change the API.

The question is whether you would like multiple small PRs to be merged, or to review on huge PR. In the latter case @bsipocz can create that from her passing branch (see test results above).

StuartLittlefair · 2017-02-18T14:47:48Z

@eteq @bsipocz one issue remaining building the docs which seems to be that astropy=1.3 is not available for numpy 1.12 on conda. is it ok to change travis.yml to use numpy 1.11 for that build or should this be fixed elsewhere?

bsipocz · 2017-02-18T16:10:21Z

That should be working and use pip install this case when an astropy version is not available. I'll look at it after I get home tonight

…

On Feb 18, 2017 2:47 PM, "StuartLittlefair" ***@***.***> wrote: @eteq <https://github.com/eteq> @bsipocz <https://github.com/bsipocz> one issue remaining building the docs which seems to be that astropy=1.3 is not available for numpy 1.12 on conda. is it ok to change travis.yml to use numpy 1.11 for that build or should this be fixed elsewhere? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#285 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGeUwivywyiwLDgjsVzE8bSVWR3W_ospks5rdwSVgaJpZM4L06KH> .

bsipocz · 2017-02-19T17:45:08Z

OK, so the problem there is that there is no wcsaxes build for np 1.12.
For the sake of this PR, yes please change NUMPY_VERSION for the docs build to 1.11.

In the meantime I'll open another PR that changes to use wcsaxes from astropy when possible rather than the standalone version.
But as wcsaxes is already been merged into astropy and s astroplan already requires astropy 1.3, I'm opening a new PR that does the switch.

…ring astropy 1.3 everywhere

bsipocz · 2017-02-19T18:19:09Z

At the end wcsaxes isn't directly used so it made sense to open a PR against this one.

Use wcsaxes from astropy

StuartLittlefair · 2017-02-20T07:29:14Z

Thanks for the PR @bsipocz In the end it hasn't helped - we now have a similar problem with astro query.

bsipocz · 2017-02-20T09:50:34Z

@StuartLittlefair - this is indeed an annoying issue. To have a quick and dirty fix, I suggest to add conda-forge to the CONDA_CHANNELS variable. For the long term we will come up with something in ci-helpers

this is a temporary fudge until astropy-helpers can be updated

StuartLittlefair · 2017-02-21T13:42:01Z

@bmorris3 @eteq @bsipocz OK - we have a passing PR!

bmorris3 · 2017-02-23T15:25:49Z

@StuartLittlefair Thanks for all of your hard work on this one!

bsipocz reviewed Feb 2, 2017

View reviewed changes

StuartLittlefair force-pushed the no_target_lists branch from 37c3ff7 to e178a49 Compare February 2, 2017 16:23

This was referenced Feb 4, 2017

Use numpy broadcasting for altaz and constraints #286

Merged

moon_phase_angle slow for large time arrays #211

Closed

StuartLittlefair added 12 commits February 18, 2017 19:04

fixed imports

be7b92b

first pass at full broadcasting

0163c12

remove automatic gridding of targets and times

42e0281

restore old broadcasting behaviour of astroplan

0e408bd

fixes for constraints

242c321

use pytest

f0c83a6

handle None type targets

a7849b1

no count_nonzero which has no axis parameter in old numpy

a1eb54a

make sure we use integer indices

390148c

avoid np.where with old numpy

f0a8c0b

schedule docstring changes

f5a151b

use float for linewidth

90f33bf

StuartLittlefair force-pushed the no_target_lists branch from 6debb12 to 90f33bf Compare February 18, 2017 12:05

update to require astropy 1.3

ff21467

Removing mentioning of wcsaxes as it's part of astropy 1.3, and requi…

fb43e2b

…ring astropy 1.3 everywhere

Merge pull request #2 from bsipocz/wcsaxes_use_astropy

240362c

Use wcsaxes from astropy

bsipocz mentioned this pull request Feb 20, 2017

conda install doesn't work for affiliates when pip installing astropy stable astropy/ci-helpers#168

Closed

added conda-forge to CONDA_CHANNELS

737de21

this is a temporary fudge until astropy-helpers can be updated

This was referenced Feb 23, 2017

Faster alt/az calculations #194

Closed

Added a utility function to get a non-scalar SkyCoord from a list of targets #215

Closed

bmorris3 merged commit 3fca943 into astropy:master Feb 23, 2017

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

Conversation

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

Uh oh!

Uh oh!

StuartLittlefair commented Feb 2, 2017

Uh oh!

bsipocz commented Feb 2, 2017

Uh oh!

bsipocz commented Feb 2, 2017

Uh oh!

bsipocz commented Feb 2, 2017

Uh oh!

StuartLittlefair commented Feb 2, 2017

Uh oh!

bsipocz Feb 2, 2017

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Feb 2, 2017

Uh oh!

StuartLittlefair commented Feb 2, 2017

Uh oh!

bsipocz commented Feb 2, 2017

Uh oh!

StuartLittlefair commented Feb 2, 2017

Uh oh!

bsipocz commented Feb 2, 2017

Uh oh!

bsipocz commented Feb 2, 2017

Uh oh!

StuartLittlefair commented Feb 2, 2017

Uh oh!

bsipocz commented Feb 2, 2017

Uh oh!

bmorris3 commented Feb 2, 2017

Uh oh!

StuartLittlefair commented Feb 2, 2017

Uh oh!

StuartLittlefair commented Feb 9, 2017

Uh oh!

bsipocz commented Feb 9, 2017

Uh oh!

StuartLittlefair commented Feb 9, 2017

Uh oh!

bsipocz commented Feb 10, 2017

Uh oh!

StuartLittlefair commented Feb 10, 2017

Uh oh!

bsipocz commented Feb 10, 2017

Uh oh!

StuartLittlefair commented Feb 10, 2017

Uh oh!

bsipocz commented Feb 11, 2017

Uh oh!

StuartLittlefair commented Feb 11, 2017

Uh oh!

bsipocz commented Feb 11, 2017

Uh oh!

StuartLittlefair commented Feb 11, 2017

Uh oh!

StuartLittlefair commented Feb 18, 2017

Uh oh!

bsipocz commented Feb 18, 2017 via email

Uh oh!

bsipocz commented Feb 19, 2017

Uh oh!

bsipocz commented Feb 19, 2017

Uh oh!

StuartLittlefair commented Feb 20, 2017

Uh oh!

bsipocz commented Feb 20, 2017

Uh oh!

StuartLittlefair commented Feb 21, 2017

Uh oh!

bmorris3 commented Feb 23, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

StuartLittlefair commented Feb 2, 2017 •

edited

Loading