-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. | ||
|
||
""" | ||
|
||
# ================ | ||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @cdeil here: i.e. Another example of how this works can be found in The init just does |
||
|
||
obs = Observer(name='Subaru Telescope', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious about the choice of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'd say reserve |
||
longitude='-155:28:48.900', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should encourage passing in an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should re-use existing That is unless there are cases where it makes the API complicated or tedious to use ... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
@eteq, @ejeschke, all – Agreed? Or do you see a case sub-classing would be better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that having a delegate object would be better than subclassing. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry -- I still feel that this should not accept There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 I thought we agreed to use EarthLocation? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also agree There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to just accept |
||
latitude='+19:49:42.600', | ||
elevation=4163, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The input should require There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 However, I'm 100% in agreement with @adrn that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we're going to require quantities for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
import astropy.units as u | ||
import pytz | ||
|
||
obs = entity.Observer(name='Keck 1', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or named classes, e.g., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The advantage being, from IPython, one could do There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 from me on that idea too. Perhaps instead of |
||
|
||
# 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @adrn that "env" here is a bit ambiguous. |
||
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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of accepting both There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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., |
||
|
||
# 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A subtle question about 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of |
||
""" | ||
|
||
# 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should also specify an obs.sunset(date=day, angle=12*u.degree) == obs.evening_twilight_12(date=day) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 to @bmorris3's syntax here. the default I think it still makes sense to have the convenience functions for the standard twilights, though. I would suggest these wordings, though:
As a shorter alternative, maybe drop the trailing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @eteq : _civil, _nautical, etc are clearer. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
|
||
# evening (civil) twilight | ||
obs.evening_twilight_18(date=day) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two other useful sun-related methods that are missing: |
||
# 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe `moon_altaz`` instead? "position" could mean a lot of things. |
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also possibly useful: |
||
# ============== | ||
# 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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A potentially provocative question: do we need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And note that it's still possible to have There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One additional thought. If it is desirable to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I'm not sure about the name |
||
|
||
# initializing from astropy entities directly | ||
from astropy.coordinates import SkyCoord | ||
|
||
t1 = SiderealTarget(name='Polaris', | ||
coord=SkyCoord('02h31m49.09s', '+89d15m50.8s', frame='icrs')) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be handy to have an
and quickly see airmass curves for objects. Not sure... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if we should leave more provisions to prepare the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that you can always just add a Re: @adrn's original point, I think |
||
# Leaves scope for NonSiderealTarget, etc. | ||
|
||
# Convenience methods for looking up/constructing targets by name via | ||
# astroquery? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 to I see this more useful for future conditions like magnitude limits, where you need some estimate of the magnitude of the target (which |
||
|
||
|
||
# ================================ | ||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There could be a convenience way to create an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PyEphem calls it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 But if y'all don't like that, another option would be to try a more specific name like |
||
|
||
# tell me about object sm in relation to observer obs at time t | ||
# returns a CalculationResult object | ||
cr = obs.calc(sm, t) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if consensus has been reached on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this @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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Summary of comments on
|
||
|
||
""" | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we get rid of the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 All I'm saying then is: let's try and find a better name for this than 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: than: 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On initial reading, I was not entirely clear on what 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @jberlanga that something like 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, (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) |
There was a problem hiding this comment.
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 isTime(input)
and that will work fine ifinput
is aTime
or adatetime
. And thedatetime
output ofTime
is always UTC, so anywhere that you want to get back a local time it can internally just betimeobj.datetime.astimezone(self.timezone)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
As you can see, the final
Time
object has the correct UTC time even though I gave it something that is in local time.