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

Adding straw-man API for discussion #4

Merged
merged 5 commits into from
Jun 27, 2015
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
265 changes: 265 additions & 0 deletions dev/planning-example.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,265 @@
#!/usr/bin/python
# -*- coding: utf-8 -*-

# ============================
# Observation Planning Toolbox
# ============================
"""
The observation planning toolbox consists of a number of entities that
simplify the planning and scheduling of observations by abstracting much
of the boilerplate code necessary to accomplish the same thing using
core astropy or ephemerides objects. These can be thought of as common
idioms or convenience classes and functions that would be recreated over
and over by astronomers and or software engineers building telescope
scheduling software.

Important assumptions:

- It should be convenient to specify dates and times in the local timezone
of the observer.

- It should be convenient to do timezone conversions easily.

This means methods that can take dates can either take astropy
dates (as used internally) or python datetime objects (not naive, but
tagged with timezone). So dates can be passed in as timezone-aware
datetime objects and results can be easily converted between such.
Copy link
Member

Choose a reason for hiding this comment

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

Good news on this front: it looks to me like astropy.time.Time does everything exactly like you'd expect with timezones. E.g. Time(datetimeobjwithtzinfo) transparently converts to utc, so all that needs to be done with accepting times is Time(input) and that will work fine if input is a Time or a datetime. And the datetime output of Time is always UTC, so anywhere that you want to get back a local time it can internally just be timeobj.datetime.astimezone(self.timezone)

Copy link
Contributor

Choose a reason for hiding this comment

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

@eteq, do astropy TIme objects respect timezone? In other words, if I pass in a timezone tagged datetime object to initialize a Time object the time will be correctly converted to (my local time) in UTC? In pyephem at least, the timezone was not considered and the time was taken directly to be UTC.

Copy link
Member

Choose a reason for hiding this comment

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

@ejeschke - yep! Note the following:

>>> dt=datetime.datetime.now(pytz.timezone('US/Eastern'))
>>> dt
datetime.datetime(2015, 6, 10, 17, 6, 17, 506659, tzinfo=<DstTzInfo 'US/Eastern' EDT-1 day, 20:00:00 DST>)
>>> astropy.time.Time(dt)
<Time object: scale='utc' format='datetime' value=2015-06-10 21:06:17.506659>

As you can see, the final Time object has the correct UTC time even though I gave it something that is in local time.


"""

# ================
# Observer objects
# ================
"""
An observer defines a single terrestrial location and applicable
environmental attributes thereof that affect observation calculations
at that location.
"""
# Define an observer by full specification
#
# Because there may be many attributes for an observer, I suggest using
# keyword parameters for most of them.
from astroplan.entity import Observer
Copy link
Member

Choose a reason for hiding this comment

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

I think we should start with a single top-level namespace package so that users don't have to remember sub-package or sub-module names.
I.e. instead of from astroplan.entity import Observer the user would do from astroplan import Observer.

Of course we will still have many files implementing the various things ... but we can expose the the end-user functionality in the main package namespace.
Example: https://github.com/astrofrog/reproject/blob/master/reproject/__init__.py#L13

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @cdeil here: i.e. from astroplan import Observer

Another example of how this works can be found in astropy.coordinates: https://github.com/astropy/astropy/blob/master/astropy/coordinates/__init__.py

The init just does from <subpkg> import * and that brings in everything in the __all__ attribute of the subpackages.


obs = Observer(name='Subaru Telescope',
Copy link
Member

Choose a reason for hiding this comment

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

Curious about the choice of Observer vs. Observatory

Copy link
Contributor

Choose a reason for hiding this comment

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

It's basically defining a terrestrial location along with some environmental data necessary to make calculations. Could also use the name Site or Location, etc. I just prefer Observer.

Copy link
Member

Choose a reason for hiding this comment

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

I think Observer or Site (or ObservingSite?) is fine - Location is to be avoided as it might get confused with astropy.coordinates.EarthLocation.

I'd say reserve Observatory for a possible future subclass which might include extra information like telescopes, instrument configurations, etc.

longitude='-155:28:48.900',
Copy link
Member

Choose a reason for hiding this comment

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

We should encourage passing in an EarthLocation or at minimum avoid passing in strings where possible.

Copy link
Member

Choose a reason for hiding this comment

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

We should re-use existing astropy.coordinates classes, namely astropy.coordinates.EarthLocation and astropy.coordinates.AltAz, as much as possible, no?

That is unless there are cases where it makes the API complicated or tedious to use ...
@ejeschke What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, except that we need to look at where additional items are necessary, in which case it needs to be easy to either add these as "metadata" to an existing object or subclass the astropy entity. e.g. As far as I know, an astropy coordinate does not have a name, yet most observers would like to see a name plotted in an airmass plot and not the numerical coordinates. As the names of the targets will crop up all over the place in scheduling, plots, etc. it needs to be easy to associate it tightly with the coordinate.

Copy link
Member

Choose a reason for hiding this comment

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

I think in most (all?) cases we should use composition instead of inheritance.

Examples:

  • An Observer has an EarthLocation member (and extra members), instead of making it an EarthLocation subclass.
  • A Target has a SkyCoord member (and extra members), instead of making it a SkyCoord subclass.

@eteq, @ejeschke, all – Agreed? Or do you see a case sub-classing would be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that having a delegate object would be better than subclassing.

Copy link
Member

Choose a reason for hiding this comment

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

I pretty much agree with @cdeil here. Basically, I think of the initializer for these classes as providing a convenient API for creating groups of other objects.

One thing I think we want to maintain rigorously, though: it should be as straightforward as possible how the keywords map to the internally stored objects. So, e.g. for Observer, on creation the object should gain two attributes which are part of the public API: obs.earthlocation and obs.altaz. With that in mind, a third creation mechanism should be possible, like this: obs = Observer(EarthLocation(...), AltAz(...)), which of course just skips the parsing of the relevant keywords and just sets earthlocation and altaz directly.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry -- I still feel that this should not accept longitude, latitude, and elevation and we should instead just require an EarthLocation -- it makes the API cleaner, reinforces OO, highlights the functionality in astropy.coordinates, etc...

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 I thought we agreed to use EarthLocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adrn I think in my last conversations with somebody (?) I was convinced that accepting strings is "how you'd expect it to behave" and in most cases it's shorter to write. But the more I think about it, the more I want to do it your way, so that you could specify a quick example in two short lines:

location = EarthLocation(0, 0, 0)
obs = Observer(name='Water', location=location)

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

I also agree EarthLocation is better. And absolutely not "strings are expected" - it might be that strings can be parsed by EarthLocation (or Angle), but an API built on magically translating strings into other things leads to lots of confusion about what's actually going on.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to just accept EarthLocation.

latitude='+19:49:42.600',
elevation=4163,
Copy link
Member

Choose a reason for hiding this comment

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

The input should require Quantity objects for things with units.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with forcing use of units to initialize, but we need to balance with making it convenient for the common cases. I note that astropy itself allows things like strings to initialize coordinates, i.e. "10h45m20.345s"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @ejeschke – I think a user should be able to copy and paste coordinates from SIMBAD without issue. I can think of a few friends who would love to use this code but would be discouraged by feeling obligated to learn to use Quantity first.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be cleaner to accept an instantiated coordinate object -- you can, of course, pass strings in to the coordinate object to initialize. This would make the code for our new class much cleaner. But I was mainly referring to the other arguments, e.g., elevation, pressure, temperature... I imagine:

obs = Observatory(name='Subaru',
                          location=EarthLocation(lon='-155:28:48.900', lat='+19:49:42.600'),
                          elevation=1638*u.meter,
                          pressure=98*u.kPa, etc..)

Copy link
Member

Choose a reason for hiding this comment

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

As per my comment above, I think it's ok in this instance to allow both cases. Definitely we need to have the option of what @adrn suggests here, because there are some ways of creating an EarthLocation that are more complex that simply giving lat lon and elevation (i.e., creating from x/y/z or from other coordinate objects). But it also makes sense that in a lot of standard cases you just want to give a single set of keywords and leave it at that.

However, I'm 100% in agreement with @adrn that elevation, pressure, and temperature need to be quantities: they are unitful numbers, so the user should have to specify the unit - and anyway, they'll be stored with their units as part of AltAz, so you'll need to provide the unit somehow anyway. For lat/lon it's fine to allow strings: in fact, all you need to say is "latitude and longitude can be anything that EarthLocation accepts", which maps directly onto the Latitude/Longitude classes, just as the comment a few lines down mentions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're going to require quantities for pressure, I understand that we won't need a default unit, but out of curiosity – what is a unit for atmospheric pressure that people might find familiar to see in tutorials/APIs? For example, APO shows inches of mercury, but that's crazy sauce. Las Campanas reports in mbar.

Copy link
Member

Choose a reason for hiding this comment

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

No idea what people prefer...

Maybe measuring pressure in terms of (Freddie) Mercury isn't so crazy, though?

Copy link
Member

Choose a reason for hiding this comment

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

@adrn - I'm very disappointed this comment got covered up so quickly with the last set of commits ;)

pressure=615,
relative_humidity=0.11,
temperature=0,
timezone='US/Hawaii',
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's a bit dangerous to allow the timezone to be a pure string? This implicitly means we're saying "anything pytz accepts is something we'll support in the future". Instead we could say it has to be provided as a tzinfo object (like in the example on line 73)? I dunno, something to consider at least.

description="Subaru Telescope on Mauna Kea, Hawaii")

# longitude and latitude would be represented internally by astropy
# angles.Longitude/Latitude. So any value that can naturally initialize
# one of those objects could be passed as the parameter to Observer
# Other items are represented internally using the appropriate astropy
# Quantities and Units and so can be initialized similarly.

# It's also possible to initialize using astropy objects and other
# direct entities
from astropy.coordinates.angles import Longitude, Latitude
Copy link
Member

Choose a reason for hiding this comment

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

The convention is that things from Astropy should always be imported from the location shown in the docs.
In this case astropy.coordinates.Longitude and not astropy.coordinates.angles.Longitude.
The fact that the Longitude class is implemented in angles.py should be regarded an implementation detail that can change in the future ...

import astropy.units as u
import pytz

obs = entity.Observer(name='Keck 1',
Copy link
Member

Choose a reason for hiding this comment

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

Just a consistency nitpick: shouldn't the way Observer is called here be the same as in Line 44?

Copy link
Member

Choose a reason for hiding this comment

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

good point - actually I suppose this wouldn't work at all given that entity is not imported anywhere above.

This is the sort of thing that usually gets sorted out when the API gets turned into a test, but certainly it doesn't hurt to get it right the first time!

longitude=Longitude(155.4750 * u.degree),
latitude=Latitude(19.8264 * u.degree)
elevation=4160 * u.m,
pressure=615 * u.hPa,
relative_humidity=0.11,
temperature=0.0 * u.C,
timezone=pytz.timezone('US/Hawaii'),
description="W.M. Keck 1 Telescope on Mauna Kea, Hawaii")

# It would also be desirable to be able to have a small database of
# common telescopes. Maybe this can at first simply take the form of
# a python module:
from astroplan import sites

obs = sites.get_observer('keck1')
Copy link
Member

Choose a reason for hiding this comment

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

Or named classes, e.g., sites.Keck1?

Copy link
Member

Choose a reason for hiding this comment

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

The advantage being, from IPython, one could do sites.<tab> and get a list of available sites.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 for named classes. I think this package is likely to be tinkered with in iPython for quick plotting, and facilitating ease of interactive use is a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

👍 from me on that idea too. Perhaps instead of sites being a module, it could be a dictionary-like object that's extended to allow attribute-style access? I.e. sites['keck1'] and sites.keck1. It's pretty straightforward to make that work with tab-completion - the magic trick is to override __dir__ as discussed here.


# Environmental conditions should be updatable. Suggest using keyword
# parameters (this would make it easy to update an internal AltAz coordinate):
#
obs.set_env_conditions(pressure=600 * u.hPa, relative_humidity=0.2,
Copy link
Member

Choose a reason for hiding this comment

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

I'm always in favor of verbosity (though others disagree), so I would call this something like set_environment() or set_conditions() or something like that...

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @adrn that "env" here is a bit ambiguous. set_conditions seems pretty clear to me, actually...?

temperature=10 * u.C)

# It's easy to construct date/times from the observer, since it knows about
# its timezone. These dates can be passed directly to target check methods.

# dates default to midnight if not otherwise specified
day = obs.get_date("2015-05-01")
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this is actually returning -- is this local time? Maybe I'm misunderstanding the purpose of this, but why do we need string support here? Can't we just work with datetime or astropy.time.Time objects? Also, I'm generally against methods that start with set_ and get_

Copy link
Contributor

Choose a reason for hiding this comment

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

I think most of the methods that deal with dates should accept an astropy time or a datetime object. As astropy time objects are assumed to be utc, I think we should have a convenient way to get date objects expressed in the local observer's time that can then be passed in. That's what this method does--return a timezone aware datetime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of accepting both astropy.time.Time and datetime objects – Python beginners and non-astropy experts will have seen datetime objects, and astropy users can be specific with astropy.time.Time objects.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with accepting all of these... and the implementation is trivial internally, as I mentioned above: Time(input) will behave correctly for all of time='2015-05-01',input=Time(...), or input=datetime(...). So the docs can just say "date can be anything that Time will accept" or similar.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, but I am also confused, as @adrn is, about why this method exists at all. If its only purpose is to translate local times into UTC, it probably shouldn't be there, because that's what datetime objects with pytz are for, and by adding a second way to do it we just confuse the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@eteq, the point of this method is to provide a convenient way to get a timezone aware date or datetime in the observing position. This is trivial if you are programming on top of of this api, but for end users simply planning an observation--if we don't provide a convenience method, then we force every user to write over and over the one or two line function to convert a time expressed in hours and minutes to the local observing time. I'm aware that some observers plan the observation is UTC, but I've seen many that plan in the local time, and prefer the airmass plots in the local time.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to add here that (as discussed in the Skyfield documentation), datetime doesn't take leapseconds, so for this/other reasons, I agree that such a method that takes care of some details internally is a good option.

Copy link
Member

Choose a reason for hiding this comment

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

@ejeschke @jberlanga - ah, ok, I get it now. So it's basically "convert some fixed time into whatever the time zone that's defined as local". That makes sense, but I would say this should then have a name that says that a bit more clearly: i.e., local_time, get_local_time or convert_to_local_time or something like that. I suppose there's nothing to stop the user from giving pytz.utc as the time zone if they want, anyway. (And that's maybe the most reasonable default?)


# Can get more and more specific
t1 = obs.get_date("2015-05-01 21:00")
t1 = obs.get_date("2015-05-01 21:33:41")

# default is current time
cur_time = obs.get_date()

# Dates are assumed to be in the observer's time zone unless a time zone
# specifier is given
t2_utc = obs.get_date("2015-05-01 21:33:41 UTC")

"""
We can ask about typical times of interest at this observing position
Returned dates are assumed to be in the observer's time zone for convenience.
The date parameter, if given, can be an astropy Time or a datetime instance.
If no parameter is given, the current date/time is assumed.
Copy link
Member

Choose a reason for hiding this comment

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

A subtle question about date: it's not obvious what "date" that's referring to. That is, are we asking about the dark period that's the morning of that day, or the night of that day?

I moderately common (but not universal) convention in astronomy is to say a date and implictly take that to mean "the night of that date". If we take that convention, then instead of date the keyword should be nightof or similar. However, that has a problem: what if I'm in pacific time and specify the date as '2015-05-01 01:00:00 Pacific'? A human reading that (or an equivalent datetime or Time object) would probably assume that means the morning of May 1/night of April 30, but the nightof convention would take it to be the night of May 1/May 2.

So an alternative is to have it be "the dark period closest to that date". That would require some calculation, though, so it has a performance overhead. One way to mitigate that might be to special-case "pure dates", like the string '2015-05-01' or a datetime.date object.

Another alternative is to only allow pure dates, which could go with the "night of" convention.

There might be other options, but those are the ones that come to mind for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of nightof--it is clearer.

"""

# sunset
obs.sunset(date=day)

# sunrise
obs.sunrise(date=day)

# moon rise
obs.moon_rise(date=day)

# moon set
obs.moon_set(date=day)

# evening (nautical) twilight
obs.evening_twilight_12(date=day)
Copy link
Member

Choose a reason for hiding this comment

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

Probably better to have a single method that you can pass in any arbitrary angle to, e.g. obs.evening_twilight(date=day, angle=12*u.degree) (or perhaps just twilight() with a switch for morning/evening?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should also specify an angle kwarg for sunrise/sunset/moonrise/moonset, such that

obs.sunset(date=day, angle=12*u.degree) == obs.evening_twilight_12(date=day)

Copy link
Member

Choose a reason for hiding this comment

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

👍 to @bmorris3's syntax here. the default angle would be 0, so obs.sunset(date=dat) would give regular sunset, but arbitrary twilight can be gotten be setting a different angle.

I think it still makes sense to have the convenience functions for the standard twilights, though. I would suggest these wordings, though:

  • evening_civil_twilight(date=day)
  • evening_nautical_twilight(date=day)
  • evening_astronomical_twilight(date=day)
  • morning_civil_twilight(date=day)
  • morning_nautical_twilight(date=day)
  • morning_astronomical_twilight(date=day)

As a shorter alternative, maybe drop the trailing _twilight - I think it's still unambiguous what it means with the morning_ and evening_ prefixes.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @eteq : _civil, _nautical, etc are clearer.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, now that I've looked back at this, I think my preferred short forms would be:

  • civil_evening(date=day)
  • nautical_evening(date=day)
  • astronomical_evening(date=day)
  • civil_morning(date=day)
  • nautical_morning(date=day)
  • astronomical_morning(date=day)


# evening (civil) twilight
obs.evening_twilight_18(date=day)

Copy link
Member

Choose a reason for hiding this comment

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

Two other useful sun-related methods that are missing: obs.noon(date=day) and obs.midnight(date=day).

# morning (nautical) twilight
obs.morning_twilight_12(date=day)

# morning (civil) twilight
obs.morning_twilight_18(date=day)

# what is the moon illumination?
# returns a float, which is percentage of the moon illuminated
obs.moon_illumination(date=t1)

# what is the moon altitude and azimuth?
# returns an altaz coordinate
obs.moon_position(date=t1)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe `moon_altaz`` instead? "position" could mean a lot of things.


Copy link
Member

Choose a reason for hiding this comment

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

Also possibly useful: moon_rise and moon_set.

# ==============
# Target objects
# ==============
'''
A target object defines a single observation target, with coordinates
and an optional name.
'''
from astroplan.entity import SiderealTarget

# Define a target.
t1 = SiderealTarget(name='Polaris', ra='02:31:49.09', dec='+89:15:50.8')
Copy link
Member

Choose a reason for hiding this comment

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

A potentially provocative question: do we need SiderealTarget at all? Why not just use SkyCoord directly?

Copy link
Member

Choose a reason for hiding this comment

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

And note that it's still possible to have NonSiderealTarget even without SiderealTarget. We could also add sidereal target later, once there's actual use for it (as discussed in the thread below).

Copy link
Contributor

Choose a reason for hiding this comment

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

@eteq this is again a convenience api. If we have to add the name as a separate step, then everyone has to write the function to set the name attribute into the coordinate. Also, how do we handle printing of the names in plots, etc? We would have to make it a convention to use a certain attribute name to have the target name printed in the plot. Also, as @bmorris3 pointed out, there are other interesting meta data that we might want to extensibly attach to the target, like magnitudes in various bands, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

One additional thought. If it is desirable to use SkyCoord directly (e.g. for vectorizing), then maybe we can simply provide convenience methods for creating a SkyCoord with the name attached (or whatever additional metadata). Perhaps if this is useful enough (metadata in SkyCoords) such capability should make its way into SkyCoord eventually.

Copy link
Member

Choose a reason for hiding this comment

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

@eteq @ejeschke - I can see the usefulness of differentiating between different types of targets in some way, such that, for example, a user can quickly use targets of one type as points of reference for others, for prioritization, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I hadn't thought about the name, @ejeschke - it's a good point that sometimes you want that. I think it still might be useful if a SkyCoord be allowed in any place a SiderealTarget might be (it would just get some sort of default name if the name is needed), but I see the utility of storing extra metadata like names. I actually don't think this is good to build into SkyCoord (because that starts breaking the principal of separation of concerns for a class which is already a bit dangerously "do everything"), so I do indeed now see the value of an object that's essentially "a SkyCoord with metadata."

I'm not sure about the name SiderealTarget: for one, I some users might not be sure what that is supposed to mean because it's telescope-oriented language - perhaps something more like FixedTarget? Furthermore, as it is right now, it might not actually be sidereal depending on what kind of SkyCoord is contained in it. That is, some SkyCoord objects already will "move" if you transform them from one observation time to another (i.e., if something is fixed in the ICRS frame and you move it to a geocetric frame, it will be in a different place depending on what obstime you use).


# initializing from astropy entities directly
from astropy.coordinates import SkyCoord

t1 = SiderealTarget(name='Polaris',
coord=SkyCoord('02h31m49.09s', '+89d15m50.8s', frame='icrs'))

Copy link
Member

Choose a reason for hiding this comment

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

Might be handy to have an airmass_plot(date=xx) convenience method so you can do, e.g.:

fig = t1.airmass_plot(observatory=obs, datetime=times)
fig = t2.airmass_plot(observatory=obs, datetime=times, fig=fig)
plt.show()

and quickly see airmass curves for objects. Not sure...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering if we should leave more provisions to prepare the SiderealTarget for interfacing with scheduling code. Would we create kwargs for SiderealTarget that can store magnitudes in various bandpasses or ephemerides (phase and epoch)? To return to your point on the Hangout @adrn , exoplanet observers like me and others will have other fundamental quantities that are as important to the targets' observability as their coords.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like we should make provisions for adding appropriate metadata to targets. I would note that the Condition objects (below) could easily have tests for magnitude ranges, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Note that you can always just add a name or meta attribute to a SkyCoord and it will work just fine.

Re: @adrn's original point, I think airmass_plot is better implemented as a separate function or possibly even a class, rather than as a method. Or it could be both (a la the matching functions in astropy.coordinates and the corresponding SkyCoord methods), although I prefer to avoid that unless there's a specific need (for coordinates it's that SkyCoord and lower-level objects both can be used with the functions).

# Leaves scope for NonSiderealTarget, etc.

# Convenience methods for looking up/constructing targets by name via
# astroquery?
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think a a convenience method to construct targets by name via Sesame would be nice!
Astropy has SkyCoord.from_name.
Maybe just a FixedTarget.from_name that calls SkyCoord.from_name?

Copy link
Member

Choose a reason for hiding this comment

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

👍 to FixedTarget.from_name. I like the idea of something a bit fancier down the road that integrates more with astroquery, but I think that's an easy place to start.

I see this more useful for future conditions like magnitude limits, where you need some estimate of the magnitude of the target (which from_name can't give, but things like SDSS or 2MASS queries might).



# ================================
# Condition objects, observability
# ================================

"""
Q: Can I observe this target on the night of May 1, 2015 between 18:30
and 05:30 HST for 30 minutes somewhere between 15 degrees and 89 degrees
altitude?
"""

# first we define the conditions
from astroplan.entity import Constraints
Copy link
Member

Choose a reason for hiding this comment

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

As above, it would be nicer if it were visible to users (although not necessarily actually in the same .py file) as astroplan.Constraints


# times can be passed in as strings (interpreted as for get_date()) or
# as astropy Time or datetime objects
cts = Constraints(time_start="2015-05-01 18:30", time_end="2015-05-02 05:30",
alt_min=15.0*u.deg, alt_max=89.0*u.deg, duration=30*u.min)

# define a target
Copy link
Member

Choose a reason for hiding this comment

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

Names seem a little inconsistent, and maybe we want to allow a range of airmasses (crazy, I know, but maybe better to have more flexibility?). What about TimeWindow, AltitudeWindow, AirmassWindow? Just brainstorming...

Copy link
Member

Choose a reason for hiding this comment

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

👍 to AltitudeWindow. I think AirmassWindow isn't necessarily as useful, though, because AltitudeWindow mostly duplicates that. I can think of no case where the most natural way to think about it is that you want your target below a certain airmass. Definitely below cerain altitudes makes sense, but then the AltitudeWindow is more natural.

Copy link
Member

Choose a reason for hiding this comment

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

There could be a convenience way to create an AltitudeWindow from airmass limits and also to get that airmass limits back. I'm not sure what the best API for something like this is. Maybe a staticfunction AltitudeWindow.from_airmass and then an airmass_limits property? Or just autility function to convert back and forth between airmass and altitudes that's separate from the AltitudeWindow class?

Copy link
Member

Choose a reason for hiding this comment

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

I see AboveAirmass as a useful possibility because it's easier to find - if I'm a python novice (or just in a hurry), I might not thing to look on AltitudeWindow if what I'm thinking I want is that "the target be above airmass of 2". But it is true that it's duplicating functionality that's really effectively the same thing. Maybe AboveAirmass can be a subclass of AlititudeWindow that has the upper altitude fixed to 90 and the bottom set by airmass?

tgt = SiderealTarget(name='S5', ra='14:20:00.00', dec='48:00:00.00')

# returns an object with information about the observability with these
# constraints
info = obs.observable(tgt, cts)

'''
attributes of return object:
observable: a boolean -- whether target is observable at desired constraints
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this is out of scope or a bit high level, but how about an additional method to tack onto observable() that suggests alternate observations in the case that info = false (eg., time window that starts a bit earlier or later during the day chosen). It could be called suggest, or suggest_time/suggest_alt/suggest_dur to break it up and look at changing just one constraint. Could work something like info = obs.observable(tgt, cond).suggest_alt(tolerance).

Copy link
Contributor

Choose a reason for hiding this comment

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

If you call observable() with two times bracketing the night, you will get the time that the target first rises above the desired elevation/airmass (> t1) and the time that it sets according to the same criteria (< t2).

rise_time: a datetime -- first time target is visible in at these conditions
set_time: a datetime -- time target is no longer visible at these conditions

Use of a return object means that additional items can be added in the
future without breaking the API (e.g. subclasses are free to attach
additional items). It can be also useful to attach an "explanation" for why
something cannot be observed.
'''

# same question, but specify the lower elevation bound to be the
# telescope minimum and I will specify an airmass of 1.2 for the target
cond = Constraints(time_start="2015-05-01 18:30", time_end="2015-05-02 05:30",
alt_min=15.0*u.deg, alt_max=89.0*u.deg, duration=30*u.min,
airmass=1.2)
info = obs.observable(tgt, cond)

'''
Suggest that Constraints implement a time range, an altitude range and an
airmass limit at beginning. Other things that could be added later:
- azimuth range
- airmass range

Idea is that Constraints can be subclassed to add additional constraints.
'''

# ======================================================
# Other useful calculations wrt an observer and a target
#=======================================================

# calculate the distance in alt and az degrees between two targets at
# the given time (e.g. to calculate slew time)
sf = SiderealTarget(name='Sf', ra='09:40:00.00', dec='43:00:00.00')
sm = SiderealTarget(name='Sm', ra='10:30:00.00', dec='36:00:00.00')
t = obs.get_date("2015-05-01 23:00")

# returns a vector or other astropy quantity specifying a delta in alt, az
obs.distance(sf, sm, t)
Copy link
Member

Choose a reason for hiding this comment

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

Probably shouldn't name it distance because this is an angular separation, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Name can be flexible, but this gives the delta in az and el from the observer's position necessary to calculate slew time between targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PyEphem calls it separation(), we could be consistent with it.

Copy link
Member

Choose a reason for hiding this comment

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

astropy.coordinates uses separation to mean "angular distance between two coordinate objects", so that's a bit problematic an that it's got an overloaded meaning. Similarly, "distance" has the specific meaning of a physical distance, Anything as simple as "distance" or "separation" has these problems, really.

And that sort of ambiguity is why I'm actually not convinced this should exist at all... This goes back to a discussion we had with astropy.coordinates: if you want the delta-RA and delta-Dec between two coordinates, you could imagine something like dra, ddec = c1.deltaradec(c2). But it turns out that some people will take that to mean c1-c2 and others will think it's c2-c1. So instead we decided that expecting users to do dra, ddec = c1.ra - c2.ra, c1.dec - c2.dec wasn't too onerous. Similarly, this could be implemented as something like dalt = obs.altaz(sf, t).alt - obs.altaz(sm, t).alt, with less "magic" under the hood and repetition.

But if y'all don't like that, another option would be to try a more specific name like obs.daltaz(sf, sm, t)


# tell me about object sm in relation to observer obs at time t
# returns a CalculationResult object
cr = obs.calc(sm, t)
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the name calc - it's unclear what it's calculating or why. Perhaps observing_parameters or something like that (although this is rendered moot if we go with the proposal I'll be making in the comments)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if consensus has been reached on calc(), so I've left it alone for more discussion in this iteration.

Copy link
Member

Choose a reason for hiding this comment

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

I think this calc questions is the biggest point we need to settle?

@bmorris3 - Could you try to spell out the options and maybe even briefly list pros / cons if there are any?

I think very explicitly listing the options and (briefly) the implications on what typical uses would look like helps to reach a good decision. This could even remain in the API proposal at the end, with the rejected API in an "alternatives considered" section. I think that's quite common in Python PEPs and Astropy APEs for important decisions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Summary of comments on calc

On the name:

  • @eteq : The name is unclear what it's calculating or why.
  • @jberlanga : maybe rename to observe, or observability?
  • @cdeil : Let's try and find a better name for this than CalculationResult.
    Maybe Observation?

Pros:

  • @ejeschke : This approach avoids a stateful approach like pyephem uses, which complicates the code using it excessively. The calculation result basically captures the relationship between a target, an observer and a time -- and allows you to calculate only those values that you need at your leisure. Suppose I want to evaluate 400 targets wrt 12 observing positions at time t1, and then just get the airmass and parallactic angle of these. With CalculationResult you just run this obs.calc(tgt, t) method the 400*12 times and extract cr.airmass and cr.pang. No extraneous calculations will be made, and nothing stateful is changed in the observer, target or time objects.
  • @ejeschke : Convenience/preference: If I am an observer doing some simple scripting, I'd much rather write: r = obs.calc(sm, m, t); r.airmass; r.pang than am = airmass(obs, sm, t); pang = parallactic_angle(obs, sm, t)
  • @ejeschke : If we want to develop for interactive users then we need to provide convenient shorthand ways of expressing things or else we leave it to someone else to write that layer

Cons:

  • @eteq : "I'm not a big fan of a 'do everything' sort of method." (Comment from Brett: But this might be a misunderstanding – calc instantiates a new object rather than calculating everything)
  • @eteq : I still think it's important that users be able to provide their own sorts of things to calculate within the framework, rather than forcing them to only have what we give them. I was going to propose that we design an API for how users could add new things to calculate, but as I thought about it it always ended up basically just "provide a function to do the calculation". So that makes me think it's better just to use the function itself.


"""
Note use of CalculationResult object--most computations are lazily delayed
until they are requested from the object. This saves a lot of time if many
objects are evaluated wrt a given time and observation position and we only
need some of the calculation results (e.g. for scheduling). For example, we
might evaluate 400 targets against a given time slot and we are only
interested in their altitude and airmass--calculation time dominates when
scheduling from a large number of potential targets.

CalculationResult objects contain a number of properties, most of which
Copy link
Member

Choose a reason for hiding this comment

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

Can we get rid of the CalculationResult object?
Expose the properties on existing objects in this API spec or invent new ones that correspond to objects that are "more real" than a CalculationResult?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm flexible on this, but want to avoid a stateful approach like pyephem uses because it complicates the code using it excessively. The calculation result basically captures the relationship between a target, an observer and a time--and allows you to calculate only those values that you need at your leisure.

Suppose I want to evaluate 400 targets wrt 12 observing positions at time t1, and then just get the airmass and parallactic angle of these. Can you suggest how you would do that with your approach? With CalculationResult you just run this obs.calc(tgt, t) method the 400*12 times and extract cr.airmass and cr.pang. No extraneous calculations will be made, and nothing stateful is changed in the observer, target or time objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be lovely if this calculation could be done lazily as an array operation. This is the one that is at the heart of most scheduling systems.

Copy link
Member

Choose a reason for hiding this comment

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

I ... want to avoid a stateful approach like pyephem uses because it complicates the code using it excessively

The calculation result basically captures the relationship between a target, an observer and a time

I haven't thought about it much, but probably yes, a new object that captures the relationship would be better (more modular) than exposing all calculations as attributes and methods on Observer and Target.

All I'm saying then is: let's try and find a better name for this than CalculationResult.
Maybe "Observation"?

I've had a quick look at Skyfield:

>>> from skyfield.api import earth, mars, now
>>> type(now())
<class 'skyfield.timelib.JulianDate'>
>>> type(earth)
<class 'skyfield.jpllib.Earth'>
>>> type(earth(now()))
<class 'skyfield.positionlib.Barycentric'>
>>> type(earth.topos('42.3583 N', '71.0636 W'))
<class 'skyfield.positionlib.Topos'>
>>> type(earth.topos('42.3583 N', '71.0636 W')(now()))
<class 'skyfield.positionlib.ToposICRS'>
>>> type(earth.topos('42.3583 N', '71.0636 W')(now()).observe(mars))
<class 'skyfield.positionlib.Astrometric'>

If I understand correctly, the equivalent to this CalculationResult is called Astrometric, which I guess is more descriptive, but I still find the name cryptic.

Copy link
Member

Choose a reason for hiding this comment

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

I may be missing something here, but I don't understand why this makes more sense than dividing the calculations up into individual functions or methods? That is, in @ejeschke case of 400*12 airmasses, why not instead do airmass(obs, sm, t)? That seems the laziest possible way to do it... (here I mean lazy in a good way ;)

Copy link
Member

Choose a reason for hiding this comment

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

To add just a bit more on that with some more thought: right now if a user wants to add something to be calclulated in this framework, they have to subclass Observer (and override calc). I was going to propose that we design an API for how users could add new things to calculate, but as I thought about it it always ended up basically just "provide a function to do the calculation". So that makes me think it's better just to use the function itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

This comes again to the convenience factor. If I am an observer doing some simple scripting, I'd much rather write:
r = calc(sm, m, t)
r.airmass
r.pang
r.ha

than:
am = airmass(obs, sm, t)
pang = parallactic_angle(obs, sm, t)
ha = hour_angle(obs, sm, t)

Again, for users using the api to write layers of software on top this is not a big issue, since they will write their own convenience methods. I guess this raises the question of for what range of users are we are targeting this package? My feeling was that the package could be used interactively or in an ipython notebook for simple observation planning as well as for writing sophisticated layers on top. If we want to include the former users then we need to provide convenient shorthand ways of expressing things or else we leave it to someone else to write that layer.

Copy link
Member

Choose a reason for hiding this comment

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

On initial reading, I was not entirely clear on what <calc(sm, t)> was meant to give. If a CalculationResult holds the results of observing a body, sm, from some site at a given JD, t, then it seems more intuitive to <observe(sm,t)> and produce an Observation, as @cdeil suggests.

P.S. My understanding of Skyfield's Astrometric is that it contains (what you need to get?) position (either via xyz or Ra/Dec/distance) and velocity. The type() output seems to exclude the potential information available.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @jberlanga that something like observe (or maybe observability?) is better than calc.

But as for the "convenience" point @ejeschke makes, I guess I'm not convinced the second case is that much harder, and I think it makes the code much more explicit, which in my view is often more important. But this is probably better discussed in the impending telecon.

represent inter-related calculations that are not computed until necessary.
These are exposed via @property attributes:

cr.airmass -- airmass at observed position
cr.pang -- parallactic angle
cr.ha -- hour angle
cr.ut -- time of calculation result in ust
cr.lt -- time of calculation result in local time
cr.gmst -- time of calculation result in gmst
cr.lmst -- ditto local mean sidereal time
cr.moon_sep -- moon separation from the target
cr.altitude -- altitude of the object
cr.azimuth -- azimuth of the object
Copy link
Member

Choose a reason for hiding this comment

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

altitude, azimuth, and airmass could be combined into a single attribute that's an altaz coordinate object. It has all of those as available attributes.

Similarly, ut and lt I think are better treated as a single object (perhaps a datetime, perhaps an astropy.time) that can be converted to the two different representations as needed.

(although see my comment above about maybe not needing these at all - as the above illustrates, some of the functionality is already wrapped up in something else)

"""

# we can also ask for a calculation via the target (same result)
cr = sm.calc(obs, t)