Adding straw-man API for discussion

[bmorris3]

No description provided.

[coveralls]

Coverage remained the same at 100.0% when pulling abcfaa8 on bmorris3:master into d75a31e on astroplanners:master.

[adrn]

Curious about the choice of Observer vs. Observatory

[ejeschke]

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.

[eteq]

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.

[cdeil]

@bmorris3 Could you please look through the skyfield and pyephem docs (or any other packages / tools you like) and see if there is functionality you think we should have that's currently not mentioned here?

[cdeil]

@ejeschke You mentioned that for Subaru scheduling you need some computations to be fast.
This might be controversial (as being prematurely thinking about optimization and thus evil), but I think it might be useful to already mention this as a comment on the respective function or via a few explicit use cases. E.g. it could be that certain parameters for certain functions are presumed scalars in our API, and then it's bad if we later realise we want to support arrays so that certain applications can be simple (no loops) and fast.

[cdeil]

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?

[ejeschke]

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.

[ejeschke]

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.

[cdeil]

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.

[eteq]

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 ;)

[eteq]

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.

[ejeschke]

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.

[jberlanga]

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.

[eteq]

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.

[ejeschke]

I think we should look wherever possible to support array operations.

This would be a huge, huge win for queue scheduling. And I'm thinking of
those LCOGT guys who spend 14.5 minutes calculating the next 15 minutes of
observation...

~E

On Sun, May 10, 2015 at 11:39 AM, Christoph Deil notifications@github.com
wrote:

@ejeschke https://github.com/ejeschke You mentioned that for Subaru
scheduling you need some computations to be fast.
This might be controversial (as being prematurely thinking about
optimization and thus evil), but I think it might be useful to already
mention this as a comment on the respective function or via a few explicit
use cases. E.g. it could be that certain parameters for certain functions
are presumed scalars in our API, and then it's bad if we later realise we
want to support arrays so that certain applications can be simple (no
loops) and fast.

—
Reply to this email directly or view it on GitHub
#4 (comment)
.

Eric Jeschke

[bmorris3]

@cdeil – Assuming we use JPL HORIZONS or similar to compute the ephemeris of the Moon, we could also build in objects to compute the ephemerides for other solar system bodies, as is done in PyEphem.

[cdeil]

@bmorris3 – I'm not sure it's a good idea to add other solar system bodies. @eteq – Probably this belongs in Astropy core or a separate affiliated package? And temporarily adding the planets to the astroplan API would just create extra work? Or maybe it does make sense to add them behind a small wrapper layer that for now uses pyephem or skyfield and then get's changed to use real Astropy classes / functions once they become available?

[bmorris3]

@eteq – There was a lot of discussion in my Hangout with @cdeil and @ejeschke on ensuring that we can vectorize calculations on lists of targets (like calculating airmass or the rise time). As an example, I've begun experimenting with calculating rise/set times with astropy in this notebook (note: this code is just for familiarizing myself with the calculation with astropy), and I'm curious whether or not operations inside the guts of astropy.utils.iers, like computing astropy.time.Time.sidereal_time(), are/can be vectorized.

Some of my apprehension which leads me to ask for your input comes from seeing the number of issues regarding IERS predictions.

[eteq]

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.

[ejeschke]

I think we should not do timezone inference. The earth location of the telescope could be very far from the observer (observing remotely, robotically, etc) and planning can be done using a different timezone as reference. Better to assume UTC unless it is overridden explicitly.

[cdeil]

+1 to UTC as default.

[adrn]

Makes sense to me!

[bmorris3]

@eteq @cdeil When I make FixedTarget by subclassing SkyCoord, the attribute name is already assigned (and it looks like it returns the string for the name of the frame). Should we consider a different keyword for the name, like object (probably not the best choice)?

[cdeil]

Why sub-class? Has this been discussed before?

"A target has a sky coordinate" suggests that composition (a SkyCoord member) could be the right object-oriented design.
Usually sub-classing is used for is a relationships, but "A target is a sky coordinate" isn't quite right, no?

[bmorris3]

I think it's been mentioned as an option, but not thoroughly discussed. It makes sense to (naive) me because the most fundamental operations we'll do on FixedTargets are operations on the coordinates.

I'm imaging the FixedTarget as a thin subclass with extra keywords, like this:

class FixedTarget(SkyCoord):
    def __init__(self, *args, **kwargs):
        # Pull out FixedTarget specific keywords before passing onto SkyCoord
        self.object = kwargs.pop('object', None)
        self.metadata = kwargs.pop('metadata', {})

        # Pass remaining keywords on to initialize SkyCoord
        SkyCoord.__init__(self, *args, **kwargs)

If the Fixedtarget class is a top level class, I think almost every operation we'd code would have to be on a .coord attribute, which seems less clean to me.

[cdeil]

@bmorris3 – OK, I can see how for FixedTargetit's appealing to subclass SkyCoord to avoid repetitive target.coord.. On the other hand, if target.name is not the target name, that's also not nice. A mixed option might be to have a target.coord member, but then re-expose most or some of it's attributes on the target class.

Basically I'm just guessing here what a good design could be ... @eteq, @adrn, @ejeschke ?

[adrn]

I would recommend not trying to subclass SkyCoord -- there is a lot of magic that is probably better not understood for the time being...

I also don't think we need to expose coord-level methods/attributes to Target -- aren't namespaces good?

[bmorris3]

More fuel for the conversation – right now the FixedTarget class doesn't do very much more than the equivalent SkyCoord other than holding a name, and potentially in the future holding mags and epochs/periods. Will FixedTarget require enough supporting code to necessitate its own class?

[eteq]

I think I agree that this should not subclass from SkyCoord - as @cdeil said it has a coordinate, rather than is one.

Put another way, we say "The thing I want to target is Polaris. It is a star with the name Polaris, and it is located at 2h31m/89d15m." One generally does not say "I am targeting a location on the sky that has the name Polaris".

More broadly, we expect there will be other kind of targets down the road that will not be subclasses of SkyCoord (i.e.targets that follow the path of the moon and hence their SkyCoord changes a lot over the course of the night). So it might be confusing for some to have coordinates and others to be coordinates.

So I'd say @bmorris3 is right that in most cases just storing a name doesn't justify a class, but here we have plans for future classes that justify it better.

[bmorris3]

Agreed, I think we're settled on this. Especially now that I see that @jberlanga had implemented FixedTarget as a subclass of a Target super class.

[ejeschke]

Chiming in late here, but completely in favor of making the skycoord an attribute of the target class, rather than a subclass.

[eteq]

On the timezone inference question @adrn brought up: I agree it's best for the default to be UTC. But perhaps we should support this but in an optional sense? E.g., have UTC be default, but allow timezone='bylocation' or something like that tell the initializer to do the kind of magic @adrn is suggesting?

[eteq]

Also, I think we're getting pretty close to converged on this. I wonder if we should go ahead and merge once the last few things are dealt with? There can always be later PRs to amend this. (apologies if this came up at the telecon and I missed it...)

[bmorris3]

@astroplanners/astroplanners-contributors I'm about to do push my last (hopefully?) iteration, and want your opinion one more time. I want to change all of the date keywords in the sunrise/sunset/moonrise/moonset, etc methods to time keywords because they're Times. Any objections?

[bmorris3]

@eteq @cdeil I'm about to change the time boundaries accepted by TimeWindow be instances of astropy.time.Time rather than strings. Sound good?

[ejeschke]

No objection to this.

On Fri, Jun 19, 2015 at 11:03 AM, Brett M. Morris notifications@github.com
wrote:

@astroplanners/astroplanners-contributors
https://github.com/orgs/astroplanners/teams/astroplanners-contributors
I'm about to do push my last (hopefully?) iteration, and want your opinion
one more time. I want to change all of the date keywords in the
sunrise/sunset/moonrise/moonset, etc methods to time keywords because
they're Times. Any objections?

—
Reply to this email directly or view it on GitHub
#4 (comment)
.

Eric Jeschke

[eteq]

👍 on the date -> time change

[eteq]

very minor, but I think airmass_plot is no longer needed?

[eteq]

@bmorris3 - I left two minor comments (feel free to just update immediately to reflect them if you agree). Other than that, I think this is good to merge!

[cdeil]

change the time boundaries accepted by TimeWindow be instances of astropy.time.Time rather than strings. Sound good?

Will it still be nice and simple to instantiate TimeWindow objects? Can you give examples how it would look? So is it always TimeWindow([Time(...), Time(...)]) or are there shorter ways, like accepting a length-2 time?

I think it would be nice if this remains simple, but it's a small detail and I don't feel strongly about it, so OK with me if you just change this to what you think is best for now.

[cdeil]

I wonder if we should go ahead and merge once the last few things are dealt with?

I always assumed we would just leave this PR open until everything here has been implemented, and then close it without merging. But as far as I remember we haven't discussed this, and if someone prefers merging it, fine with me. (I don't see any advantage to merging it, and the slight disadvantage of having a code examples in the repo which very likely won't work in a few months, so better to just copy & paste from here and include it's contents over time into the code / tests / docs directly?)

[bmorris3]

@cdeil An example of how it would look is written in the API above. I agree that it should be kept simple, but I think it would be most flexible if instances of Time were the inputs, since such nice parsing machinery is built into them. I like the idea of a length-two time object as an alternative.

And in reference to merging – I don't mind merging or not, I'll just have to figure out a more permanent solution to my mistake of writing the API on my master branch if we don't merge :)

[jberlanga]

@bmorris3 That makes sense to me.

[cdeil]

Please bear with me ... I'm a gamma-ray astronomer that never used airmass. I know what altitude and zenith angle is.

What does "above airmass 1.2" mean?
I presume usually the constraint people want is to observe at low zenith angles = high altitudes, right?
And that corresponds to low airmass, so usually people want an upper limit as an airmass constraint, no?
https://en.wikipedia.org/wiki/Air_mass_(astronomy)#/media/File:AirmassFormulaePlots.png

But in this API spec and at http://astroplan.readthedocs.org/en/latest/api/astroplan.AboveAirmass.html we only allow to specify a lower limit.

Here's what I think:

• The relation of airmass and altitude should be explained in the airmass constraint docstring
• The user should be able to constrain airmass low / high, just like it's possible for altitude. The default could be None for both, meaning no constraint on either min or max?
• I would prefer something regular for min / max constraints ... use TimeRange, AltitudeRange and AirmassRange. (at the moment we have AboveAirmass, AltitudeRange, TimeWindow.

[ejeschke]

Sorry, bad wording on my part. By "above airmass 1.2" I meant "at a better airmass" (conflating with elevation in my attempt to be concise). +1 for TimeRange, AltitudeRange, and AirmassRange.

[eteq]

To close the loop on this, a recent change made this AirmassRange, but it can still be used in the single-argument form to mean "at this airmass or better".

[eteq]

@bmorris3 @cdeil - re: accepting time: how about saying that wherever times are used they'll be passed into the constructor of Time? That is, TimeWindow would be implemented as:

class TimeWindow(Constraint):
    def __init__(self, t1, t2):
        self.t1 = Time(t1)
        self.t2 = Time(t2)

Note that Time(Time(...)) does just what you'd expect: generates a copy of the inner Time object. So this could then either accept Time objects (the only way to get full flexibility including different time scales or whatever), or just '2010-1-1' would work because Time parses that automatically. Then you also don't have to document what it accepts beyond saying "see the Time docs for valid formats for this input".

[cdeil]

+1 to @eteq's suggestion of passing TimeWindow constructor inputs along to the Time constructor.
We could mention again in the docstring that this will give scale='utc' and format='iso':

>>> Time('2010-1-1')
<Time object: scale='utc' format='iso' value=2010-01-01 00:00:00.000>

[eteq]

Just so we're al on the same page, I think everything has been addressed here, right? So this is probably good to merge unless someone has final bits to add at the end of the day.

[bmorris3]

Reminder: @astroplanners/astroplanners-contributors It looks like we're good to merge this one too.