HiveBrain v1.2.0
Get Started
← Back to all entries
snippetpythonMinor

Generate iCalendar .ics files with events for astrological aspects

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
icalendarastrologicaleventswithgeneratefilesaspectsforics

Problem

I'm relatively new to Python, coming from a deep C++ background. I'm mostly looking for feedback on how to make my code more idiomatic/pythonic, but I would welcome and appreciate any and all other feedback as well.

This code uses the Python library PyEphem, which builds upon XEphem, an X (UNIX GUI) program that's an astrological ephemeris, which is (was historically) a table of celestial bodies and their positions in the sky. PyEphem is used to track the altitudes of astronomical bodies, which are used as an indication of when astrological aspects have or are going to occur.

I'm deploying this code as an AWS Lambda microservice, thus the lambda_handler function. Consider also reviewing the test and deployment Makefile that corresponds to the module below.

Here is the entirety of my module:

```
#!/usr/bin/env python2.7

# aspectus: aspect+prospectus. iCalendars with astrological aspect events.
# Copyright (C) 2017 Frederick Eugene Aumson
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU Affero General Public License as published
# by the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU Affero General Public License for more details.
#
# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see .

"""aspectus: aspect+prospectus. Generates iCalendar .ics files to populate your
calendar application with events for astrological aspects (angles) between two
celestial bodies. Currently supports only the trine and sextant aspects, and
only between the sun and the earth. Could easily be extended to support other
bodies, and perhaps with not much more work extended to support aspects between
two bodies outsi

Solution

Just reviewing the function find_altitude.

  1. Review



-
The docstring is comprehensive, which is good.

-
But the specification is very complicated, which makes the function hard to use. In particular, the caller has to remember to set observer.date to the start of the span to be searched, and to call body.compute(observer). It would be easy to forget to do one or both of these, and then the function would go wrong. It would be simpler if find_altitude did both of these tasks. This would require adding an extra parameter giving the start of the period to be searched.

-
In Python, it is conventional to use the names start, stop, and step (in that order) when describing an arithmetic progression: see for example the arguments to range and slice.

-
When a function needs to return multiple results (as here, where you have an altitude and a datetime) it is usually most convenient to return them as a tuple (rather than a dictionary). That's because the caller can conveniently use tuple unpacking to assign the results to local variables, like this:

altitude, datetime = find_altitude(...)


If you like having a result with named fields, then you can use a collections.namedtuple, which would give you the best of both worlds.

-
I think altitudes would be a better name than targets.

-
In the case where none of the altitudes is crossed, it would be a good idea to raise an exception rather than returning None. The reason is that it would be easy for the caller to forget to check for the exceptional case, possibly leading to incorrect results, whereas an exception is unambiguous.

-
Formatting the docstring into paragraphs would improve its readability. See below for how I would write it (though you might need to mark it up with :param: and whatnot).

-
Here you go through a rather winding path to convert body.alt to degrees, by converting it to a string representation, parsing that as a float, and then converting that to degrees:

altitude = degrees(float(repr(body.alt)))


It would be better to take a shorter path:

altitude = degrees(body.alt)


But an alternative approach would be to convert all the target altitudes to radians once on input, avoiding the need for further conversions during the search.

-
Here you compare altitude with each target altitude in turn, taking \$Θ(n)\$ if there are \$n\$ target altitudes:

before = [target < altitude for target in targets]


But if the target altitudes were sorted into numerical order, then you would only need to know the position of altitude in the sorted list, which can be found in \$O(\log n)\$ using bisect.bisect. If the position in the sorted list changes, then you know that body has crossed one (or more) of the altitudes.

-
Each time around the loop you convert observer.date to a datetime twice:

while True:
    # ...
    observer.date = observer.date.datetime() + step
    if observer.date.datetime() > latest_date:


but it would be simpler to convert the inputs into PyEphem data structures once, and then no conversions would be required in the loop:

date = ephem.Date(start)
stop = ephem.Date(stop)
step = step.total_seconds() * ephem.second

while True:
    # ...
    date += step
    if date > stop:
        raise NotFound()
    observer.date = date


-
Once you've found a timestep in which the body has crossed a target altitude, then you can use PyEphem's newton function to efficiently find the exact crossing point. This uses the Newton-Raphson method to get quadratic convergence (the number of correct digits roughly doubles in each step), whereas the the approach in the post only has linear convergence.

  1. Revised code



```
import ephem
from bisect import bisect
from math import radians

class NotFound(Exception):
pass

def find_altitude(altitudes, body, observer, start, stop, step):
"""Find the first altitude crossed by a body in a time period.

Parameters
altitudes: list(float) -- the target altitudes in degrees
body: Body -- the body
observer: Observer -- observer from whose location the
altitude is calculated.
start: datetime -- start of the period to be searched
stop: datetime -- end of the period to be searched
step: timedelta -- size of the search steps

Returns a pair (altitude, datetime) whose first element is the first
altitude crossed by the body in the time period, and whose second
element is the datetime at which that happens.

If none of the altitudes is crossed in the time period, raise
NotFound.

"""
orig_altitudes = sorted(altitudes)

# Convert inputs to PyEphem data structures.
altitudes = list(map(radians, orig_altitudes))
date = ephem.Date(start)
stop = ephem.Date(stop)
step = step.total_seconds() * ephem.second

def alt(d):
# Return altitude of body as seen by observer at date d.
observer.date = d
body.compute(observer)
ret

Code Snippets

altitude, datetime = find_altitude(...)
altitude = degrees(float(repr(body.alt)))
altitude = degrees(body.alt)
before = [target < altitude for target in targets]
while True:
    # ...
    observer.date = observer.date.datetime() + step
    if observer.date.datetime() > latest_date:

Context

StackExchange Code Review Q#153958, answer score: 4

Revisions (0)

No revisions yet.