skyfielders / python-skyfield

Elegant astronomy for Python
MIT License
1.43k stars 213 forks source link

Documentation suggestion: make it clear that ts.utc() can handle any timezones #406

Closed akkana closed 4 years ago

akkana commented 4 years ago

First, THANK YOU for adding the import of comet elements from sources like the MPC! I struggled for four hours yesterday to do that in spiceypy, then I stumbled on skyfield issue #11 where you said you'd just added it, and your clear documentation got me going in no time.

But I find the time documentation pages a little confusing, https://rhodesmill.org/skyfield/api-time.html https://rhodesmill.org/skyfield/time.html

The api doc lists utc(year, month=1, day=1, hour=0, minute=0, second=0.0) but not utc(datetime). Then in the description, it mentions that it can take an aware datetime; but the example only shows an aware datetime with tzinfo=utc and doesn't mention what happens if you pass in an aware datetime object with a different timezone. In practice, any aware datetime seems to work fine. Is that just luck, or is it part of the definition? Should I be calling astimezone(datetime.timezone.utc) on my datetimes before passing them to utc() ? utc() seems to be the only function for converting from a datetime to a skyview Time (or is there a better one to use for non-utc datetimes?), so this is a very common operation and it would be great to have it clarified in the docs.

Thanks for a great package!

brandon-rhodes commented 4 years ago

First, THANK YOU for adding the import of comet elements from sources like the MPC! I struggled for four hours yesterday to do that in spiceypy, then I stumbled on skyfield issue #11 where you said you'd just added it, and your clear documentation got me going in no time.

Wonderful, I'm glad that after taking so long to finally land, the feature was just-in-time for at least one script :)

But I find the time documentation pages a little confusing …

Are you referring to the section https://rhodesmill.org/skyfield/time.html#utc-and-your-timezone specifically? Its use of eastern and its mention of EST are intended to address the question of other-than-UTC time zones. If you could read over that section again, we can improve the docs if you could share:

  1. What maneuver or calculation you want to do that's not included in that section. I can then go add it!
  2. Are there other places in the docs, like the API docs, that need to be fleshed out and to point to that section for details? It sounds like I should expand some of the docstrings to all point to the guide to time zones — but which docstrings, do you think?
akkana commented 4 years ago

You know, I think maybe part of my problem was that I spent too much time in api-time.html, which doesn't mention non-UTC timezones, and not enough time in time.html, which, you're right, does make it fairly clear that utc() is the function to use to create a non-UTC time.

However, the discussion of timezones in time.html is also a little misleading since it says you must use pytz. I'm avoiding pytz ever since hearing Paul Ganssle on talkpython.fm say that pytz is disrecommended and will probably be dropped in two years (though his blog post on why not to use pytz doesn't mention the dropping in two years part). What I'm doing in my current comet program is:

import dateutil.parser
# ...
    if args.time:
        pdate = dateutil.parser.parse(args.time)
        if not pdate.tzinfo:
            # interpret it locally by default
            pdate = pdate.astimezone()
        t = ts.utc(pdate)

So if I run comet.py -t '2020-07-10 21:15 MDT' '2020 F3' dateutil.parser makes a timezone for MDT; if I run comet.py -t '2020-07-10 21:15' '2020 F3' datetme.astimezone() interprets it as local time (which for me is MDT). Either way, it makes a tzinfo of class datetime.timezone, which skyfield seems to interpret correctly, even though I'm not using pytz at all. (Here's comet.py in case you want to try it. And feel free to make suggestions if I'm doing anything wrong; I've used pyephem a lot but skyfield only a little so far.)

So my suggestion would be:

  1. Add a note in api-time.html that utc() converts any aware datetime, whether it's UTC or not
  2. Change the part in time.html that says you need the extra package pytz, since it doesn't seem to be true and may eventually be deprecated or removed. dateutil.parser isn't required either; you can do the same thing with something like datetime(2020, 7, 10, 21, 15, 0).astimezone(), or, maybe safer (I'm still trying to figure out the best practices),
    localtz = datetime.now().astimezone().tzinfo
    t = datetime(2020, 7, 11, 21, 15, 0).astimezone(localtz)
brandon-rhodes commented 4 years ago
  1. That sounds good, I'll update the time documentation in the API docs tomorrow, and comment here when the update is done.
  2. Skyfield still supports Python 2, which has no ability on its own to provide time zones:
>>> from datetime import datetime
>>> datetime.now().astimezone()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Required argument 'tz' (pos 1) not found

Thanks for the link to the article about pytz, I'll take a look at it!

akkana commented 4 years ago

Excellent point about Python 2. I hadn't tested that. And the comment about other time zones too (weird, I got that in the email notification but don't see that here): I had naively assumed that since dateutil.parser.parse("2020-07-11 10:20:00 MDT") worked, then it would work for other time zones like EDT too, but no: dateutil knows the short abbreviation for my local timezone, but not for other timezones. However, you can get a timezone using the long form without pytz (in Python3): dateutil.tz.gettz("America/New York").

Maybe just loosen the language a little so it isn't so specific about needing pytz? There are lots of ways to get an arbitrary timezone: pytz, dateutil.tz, arrow, a new module called zoneinfo that's external now but supposedly will be part of the stdlib in 3.9, maybe others. It does make sense to mention pytz as an option, especially for python2, but "you will need to install the third-party pytz package" is isn't true even for python2 (there's at least one other third-party option, arrow -- at least it looks like arrow would work, I haven't used it myself), and python3 has several other options and doesn't require installing anything extra.

(Argh, I hate time programming, why can't we all just use UTC? And why can't the python maintainers choose one time library, stick with it and make it work well, rather than continually introducing new ways to do things?)

brandon-rhodes commented 4 years ago

And the comment about other time zones too …

I edited the comment as soon as I started reading the article, because I saw that my assertion that Python doesn't carry time zones was soon to become false. :)

Maybe just loosen the language a little so it isn't so specific about needing pytz?

I'll do that! Until you pointed out this article I had no idea that progress had been made in the Standard Library or that everyone wasn't going to use pytz forever. Within the next week or two I'll plan to rewrite and expand that documentation to talk about the different options for different Python versions. Thanks again for sharing the article so we can make the Skyfield docs more up-to-date!

brandon-rhodes commented 4 years ago

@akkana — Thanks for your ideas! The timezone docs now mention other alternatives to pytz for more modern versions of Python. And I have added constructors ts.from_datetime() and ts.from_datetimes() that make no mention of "utc" in their name and so hopefully will not create any confusion or expectation that the datetime has any particular timezone like UTC.

Feel free to comment further here in this issue if there's anything the updated docs leave ambiguous!

Note that it won't be til I make a new Skyfield release later today that the new methods are available for most users. You can go ahead and try them out now, though, with:

pip install https://github.com/skyfielders/python-skyfield/archive/master.zip

Enjoy!