Beginning in the FixedTarget and Observer classes #11

bmorris3 · 2015-06-19T17:07:59Z

No description provided.

bmorris3 · 2015-06-19T17:12:46Z

astroplan/core.py

@@ -25,8 +40,10 @@ class Observer(object):
    Some comments.
    """

-    def __init__(self, name, longitude, latitude, elevation, pressure,
-                 relative_humidity, temperature, timezone, description=None):
+    def __init__(self, name=None, location=None, latitude=None, longitude=None,


Should we instead use the **kwargs syntax and then kwargs.get or kwargs.pop the keywords one at a time?

I think my preference is no to kwargs -- if the arguments are spelled out, it is easier to quickly see what is expected from the documentation of the call sequence.

👍 to @adrn here. Also makes it easier to see what the default is.

cdeil · 2015-06-19T17:22:45Z

@bmorris3 – When you make pull requests, we get a notification, but we don't know whether you want us to review them at this point or not. In Astropy and generally on Github, people do this differently, you could:

Say whether this is ready for review or not in the description.
If it's work in progress (usually is when the PR is first made) and we shouldn't review yet, add WIP: to the title or add a task list (https://help.github.com/articles/writing-on-github/#task-lists) of things you still want to do in this PR.

Another important thing to know (which you probably do) is that Github doesn't send notifications when you push new commits to a branch. So if there has been review, and then you've addressed the comments, and want something to be merged or a second round of review, you have to leave a comment saying so.

cdeil · 2015-06-19T17:24:10Z

astroplan/core.py

+        location : `~astropy.coordinates.EarthLocation`
+            The location (latitude, longitude, elevation) of the observatory.
+
+        longitude : str or `~astropy.units.quantity`


quantity needs to be Quantity or the Sphinx link won't work.
You probably know this, but just in case ... the commands to check the docs locally are:

python setup.py build_sphinx open _docs/_build/html/index.html

Gah, I should have caught that!

eteq · 2015-06-19T18:29:35Z

@bmorris3 - something else is confusing me about this PR: it appears to have some (all?) of the commits from #4. Did you mean to do that? I'm guessing not, because it looks like this is only supposed to include some changes in core.py, and you have a commit where you removed the API doc. This doesn't actually remove it from history, and if you merge this after the API is merged, this will delete that document, which I don't think is what you want.

So maybe you want to rebase this against master.

git checkout initclasses # you might already be on this branch
git branch initclasses-backup # insurance in case you screw up something -- you'll  always back it all out by doing git reset --hard initclasses-backup
git fetch upstream  # this might actually be "astroplanners" depending on how your remotes are set up.  I try to avoid ever having an "origin"/"upstream" because I find it confusing, so I always use the same names as github assigns.  But YMMV.
git rebase -i upstream/master

this will bring up an editor showing you all the commits between master and this branch. You'll want to delete the top 3 (which are from the API branch). Then when you close your editor it will give you an error about The previous cherry-pick is now empty, possibly due to conflict resolution. Just do git rebase --continue and everything will be fine. Once you've done that, you'll see that this branch is just the two commits you wanted and nothing else. So you can do git push origin initclasses -f and all should be good.

bmorris3 · 2015-06-19T19:39:17Z

Just to leave a note here – @eteq helped me do the rebase, they won't appear in the PR.

Those were not the commits you were looking for.
– Obi Eteq

cdeil · 2015-06-20T14:44:10Z

astroplan/core.py

@@ -2,6 +2,21 @@
 from __future__ import (absolute_import, division, print_function,
                        unicode_literals)

+from astropy.coordinates import (EarthLocation, Latitude, Longitude, SkyCoord,


@bmorris3 – Do you understand this? https://travis-ci.org/astroplanners/astroplan/jobs/67568895#L372 (I don't)

I don't either, maybe @eteq can give us some hints on how to read these logs on Monday or in the Hangout on Tuesday.

I still don't understand why that line gave an error on travis-ci:

ImportError: No module named astropy.coordinates

But if you fix the other errors mentioned below and python setup.py test (which imports this file) works locally, it should work on travis-ci, too.

Just adding a line note to remind us that this was resolved by a later commit -- the issue was a missing indentation in astroplan/__init__.py for a line that was importing from astroplan.core.

eteq · 2015-06-23T21:24:41Z

A quick note: that missing indent that fixed the tests that @bmorris3 added in the last commit at @adrn's suggestion was not a problem with the package template, because there's no core module in the package template. (no action required, just an FYI about why it happened)

bmorris3 · 2015-06-24T05:59:43Z

astroplan/tests/test_core.py

+    # positions due to leap seconds.
+    tolerance = 30*u.arcsec
+    assert (abs(pyephem_altitude - astroplan_altitude) < tolerance and
+            abs(pyephem_azimuth - astroplan_azimuth) < tolerance)


@astroplanners/astroplanners-contributors : Leap seconds will make comparison with PyEphem tricky in our tests, since PyEphem uses old Earth orientation standards and astroplan uses up-to-date IERS tables. Any ideas on how to handle the differences in apparent positions of targets as a result?

Here I use a very broad tolerance in the accepted difference in altitude/azimuth between astroplan and PyEphem (30 arcsec). In the above test, the differences are significantly smaller than this tolerance:

>>> (pyephem_altitude - astroplan_altitude).to('arcsec') <Angle 0.10845603317193309 arcsec> >>> (pyephem_azimuth - astroplan_azimuth).to('arcsec') <Angle 7.0463692237353825 arcsec>

There's not much you can do. Either make the tolerance so large that it's OK (and document in a comment or somewhere that the difference is understood). Or test against another software package, e.g. does Skyfield have this functionality?

cdeil · 2015-06-24T07:47:27Z

astroplan/tests/test_core.py

+                    location=location,
+                    **environment_kwargs)
+
+    assert obs1.location == obs2.location, ('Locations not parsed equivalently')


I'm -1 on adding descriptions of assert fails.
Over time it adds up to significance work and amount of text we have to write / discuss, with IMO no benefit.
When tests fail pytest will give a very good error message (line number, values, traceback).
Usually one will have to look at the test and code anyways, and then the description on the assert adds nothing useful.

@cdeil What do you mean here? I think assert tests are useful to have for things that must be true, but haven't really thought about it...

@adrn – I'm not sure I understand what you mean with "assert fails".
All asserts in tests should always be true, no?

I mean that here , ('Locations not parsed equivalently') can be removed because it's useless.

I think I mean "assert tests"... and I read you as saying "assert tests" as well! But now I see what you mean...oops

I don't see the problem here in a general sense, @cdeil : leaving a useful note about what you're trying to test with the assert is sort of self-documenting code in my mind. You could instead put in a comment just above, but I think this works just as well.

That said, that particular string is not terribly informative in that it says something you could guess pretty much right away from the fact that it failed. I'd go with something more like “using latitude/longitude/elvation keywords gave a different answer from passing in an EarthLocation directly".

@eteq – I agree leaving a note on the purpose of a given test / assert can be useful.
So far I have been doing this in docstrings or comments in tests, usually not at the place of the assert, but higher up where I start a new test. As far as I can see that's also the common thing to do in Astropy:
https://github.com/astropy/astropy/search?p=1&q=assert&utf8=%E2%9C%93

Another disadvantage if putting this note after the assert is that often you're almost at the end of the line, and then you have to wrap a lot (see the examples below in this PR).

But my main point is that asserts, for written and debugged code will very rarely fail, and thus it's just not worth to implement extra notes for every assert. If someone has a test failure, it's always worth reporting.
The user should copy & paste the console output into the issue report or a gist, and then a developer / maintainer will look at the test code anyways.

But bottom line: this is probably partly a matter of taste, and @bmorris3, if you like it this way, we'll still merge this tomorrow latest. :-)

adrn · 2015-06-25T14:33:05Z

astroplan/core.py

-        If time zone (local or otherwise) requested, then uses `datetime.datetime`
-        and pytz.timezone to convert from UTC.
+        else:
+            raise TypeError('Observatory location must be specified with '


Make me happy, and put EarthLocation as the 1st option here :) -- we want to encourage the OOP!

Anything to make you happy, @adrn!

eteq · 2015-06-26T01:43:18Z

astroplan/core.py

@@ -22,109 +38,152 @@

 class Observer(object):
    """
-    Some comments.
+    A container for information about an observatory's location and environment.


(minor quibble) maybe "a container class" instead of just "a container"?

also, maybe "observer" instead of "observatory"? After all, it could just be a person outside with their eyes...

eteq · 2015-06-26T01:59:51Z

I left a few more comments @bmorris3, but all but the last are phrasing/polish bits on the docs, nothing substantial on the code itself. The last one is an important subtlety, though, that I think needs to be addressed (fortunately, it's easy to fix).

bmorris3 · 2015-06-26T05:06:38Z

@eteq : thanks for the comments! They've been addressed in the latest commit.

cdeil · 2015-06-26T07:08:39Z

@eteq @adrn – Does one of you have time for a final review and hitting the merge button for this PR later today?

adrn · 2015-06-26T14:56:15Z

This looks good to me -- way to go @bmorris3! I'm going to fire away and merge this.

Beginning in the FixedTarget and Observer classes

cdeil · 2015-06-26T15:13:27Z

🎈

Have a nice weekend, everyone!

bmorris3 · 2015-06-26T15:55:31Z

🎊 One of many reasons to celebrate today!

adrn · 2015-06-26T15:58:44Z

I merged this for 🌈 !

bmorris3 reviewed Jun 19, 2015
View reviewed changes

cdeil reviewed Jun 19, 2015
View reviewed changes

bmorris3 added 3 commits June 19, 2015 11:35

Filling in FixedTarget and Observer progress

15322ca

Added ra/dec methods in Target class for FixedTarget

7883eb2

adding TODOs

2d62aba

bmorris3 force-pushed the initclasses branch from 1d78682 to 2d62aba Compare June 19, 2015 18:47

cdeil reviewed Jun 20, 2015
View reviewed changes

bmorris3 added 2 commits June 23, 2015 10:10

Fixing some errors in build

4082c63

Missing indent causing travis to fail

0cddae5

bmorris3 force-pushed the initclasses branch from aa8c2c9 to 0cddae5 Compare June 23, 2015 21:49

Adding tests for astroplan.core (currently passing)

e4516e4

bmorris3 reviewed Jun 24, 2015
View reviewed changes

(Hopefully) increasing test coverage

cd215d9

cdeil reviewed Jun 24, 2015
View reviewed changes

Updates based on comments by @cdeil and @eteq

9c65d7a

adrn reviewed Jun 25, 2015
View reviewed changes

bmorris3 added 3 commits June 25, 2015 10:59

Addressing comments by @adrn

1ba6312

Adding docstrings, more descriptive test fail messages

94443af

Repairing altaz docstring

4dd152a

eteq reviewed Jun 26, 2015
View reviewed changes

Addressing comments by @eteq

615bc8c

cdeil added the enhancement label Jun 26, 2015

cdeil added this to the 0.1 release milestone Jun 26, 2015

adrn added the final-review label Jun 26, 2015

adrn added a commit that referenced this pull request Jun 26, 2015

Merge pull request #11 from bmorris3/initclasses

a3eee9a

Beginning in the FixedTarget and Observer classes

adrn merged commit a3eee9a into astropy:master Jun 26, 2015

bmorris3 deleted the initclasses branch August 10, 2015 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beginning in the FixedTarget and Observer classes #11

Beginning in the FixedTarget and Observer classes #11

bmorris3 commented Jun 19, 2015

bmorris3 Jun 19, 2015

adrn Jun 23, 2015

eteq Jun 26, 2015

cdeil commented Jun 19, 2015

cdeil Jun 19, 2015

bmorris3 Jun 19, 2015

eteq commented Jun 19, 2015

bmorris3 commented Jun 19, 2015

cdeil Jun 20, 2015

bmorris3 Jun 20, 2015

cdeil Jun 20, 2015

adrn Jun 25, 2015

eteq commented Jun 23, 2015

bmorris3 Jun 24, 2015

cdeil Jun 24, 2015

cdeil Jun 24, 2015

adrn Jun 25, 2015

cdeil Jun 25, 2015

adrn Jun 25, 2015

eteq Jun 25, 2015

cdeil Jun 25, 2015

adrn Jun 25, 2015

bmorris3 Jun 25, 2015

eteq Jun 26, 2015

eteq Jun 26, 2015

eteq commented Jun 26, 2015

bmorris3 commented Jun 26, 2015

cdeil commented Jun 26, 2015

adrn commented Jun 26, 2015

cdeil commented Jun 26, 2015

bmorris3 commented Jun 26, 2015

adrn commented Jun 26, 2015

Beginning in the FixedTarget and Observer classes #11

Beginning in the FixedTarget and Observer classes #11

Conversation

bmorris3 commented Jun 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeil commented Jun 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eteq commented Jun 19, 2015

bmorris3 commented Jun 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eteq commented Jun 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eteq commented Jun 26, 2015

bmorris3 commented Jun 26, 2015

cdeil commented Jun 26, 2015

adrn commented Jun 26, 2015

cdeil commented Jun 26, 2015

bmorris3 commented Jun 26, 2015

adrn commented Jun 26, 2015